Skip to content

Commit

Permalink
HParams: Bug Fix - Ensure context menu always opens within the window (
Browse files Browse the repository at this point in the history
…#6474)

## Motivation for features / changes
It was possible to open a context menu which would overflow the page.
This actually lead to a strange CLS issue which looked pretty bad. Now
the custom_modal component will check the size of the content once it
has rendered and adjust its position to ensure it fits in the page.

## Screenshots of UI changes (or N/A)
The context menu never overflows the window

![35353f79-2987-455b-af30-d05e1e29cefa](https://github.com/tensorflow/tensorboard/assets/78179109/aa44934c-7d51-4154-9e83-33b67d8e01b3)

This also works at the bottom of the page but it doesn't come across as
well in a screenshot

![image](https://github.com/tensorflow/tensorboard/assets/78179109/0158dbbf-2507-4ed9-92ca-a4293f94887d)
  • Loading branch information
rileyajones authored Jun 29, 2023
1 parent 5004d78 commit 7c365cb
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 0 deletions.
24 changes: 24 additions & 0 deletions tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export class CustomModalComponent implements OnInit {
// Wait until after the next browser frame.
window.requestAnimationFrame(() => {
if (visible) {
this.ensureContentIsWithinWindow();
this.onOpen.emit();
} else {
this.onClose.emit();
Expand All @@ -90,6 +91,29 @@ export class CustomModalComponent implements OnInit {
document.addEventListener('click', this.clickListener);
}

private ensureContentIsWithinWindow() {
if (!this.content) {
return;
}

const boundingBox = this.content.nativeElement.getBoundingClientRect();
if (boundingBox.left < 0) {
this.content.nativeElement.style.left = 0;
}
if (boundingBox.left + boundingBox.width > window.innerWidth) {
this.content.nativeElement.style.left =
window.innerWidth - boundingBox.width + 'px';
}

if (boundingBox.top < 0) {
this.content.nativeElement.style.top = 0;
}
if (boundingBox.top + boundingBox.height > window.innerHeight) {
this.content.nativeElement.style.top =
window.innerHeight - boundingBox.height + 'px';
}
}

@HostListener('document:keydown.escape', ['$event'])
public close() {
document.removeEventListener('click', this.clickListener);
Expand Down
56 changes: 56 additions & 0 deletions tensorboard/webapp/widgets/custom_modal/custom_modal_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {ComponentFixture, TestBed} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {CustomModalComponent} from './custom_modal_component';
import {CommonModule} from '@angular/common';
import {
Expand Down Expand Up @@ -135,4 +136,59 @@ describe('custom modal', () => {
expect(fixture.componentInstance.isOpen).toBeFalse();
});
});

describe('ensures content is always within the window', () => {
beforeEach(() => {
window.innerHeight = 1000;
window.innerWidth = 1000;
});

it('sets left to 0 if less than 0', async () => {
const fixture = TestBed.createComponent(TestableComponent);
fixture.detectChanges();
fixture.componentInstance.openAtPosition({x: -10, y: 0});
expect(fixture.componentInstance.isOpen).toBeFalse();
await waitFrame();
fixture.detectChanges();

const content = fixture.debugElement.query(By.css('.content'));
expect(content.nativeElement.style.left).toEqual('0px');
});

it('sets top to 0 if less than 0', async () => {
const fixture = TestBed.createComponent(TestableComponent);
fixture.detectChanges();
fixture.componentInstance.openAtPosition({x: 0, y: -10});
expect(fixture.componentInstance.isOpen).toBeFalse();
await waitFrame();
fixture.detectChanges();

const content = fixture.debugElement.query(By.css('.content'));
expect(content.nativeElement.style.top).toEqual('0px');
});

it('sets left to maximum if content overflows the window', async () => {
const fixture = TestBed.createComponent(TestableComponent);
fixture.detectChanges();
fixture.componentInstance.openAtPosition({x: 1010, y: 0});
expect(fixture.componentInstance.isOpen).toBeFalse();
await waitFrame();
fixture.detectChanges();
const content = fixture.debugElement.query(By.css('.content'));
// While rendering in a test the elements width and height will appear to be 0.
expect(content.nativeElement.style.left).toEqual('1000px');
});

it('sets top to maximum if content overflows the window', async () => {
const fixture = TestBed.createComponent(TestableComponent);
fixture.detectChanges();
fixture.componentInstance.openAtPosition({x: 0, y: 1010});
expect(fixture.componentInstance.isOpen).toBeFalse();
await waitFrame();
fixture.detectChanges();
const content = fixture.debugElement.query(By.css('.content'));
// While rendering in a test the elements width and height will appear to be 0.
expect(content.nativeElement.style.top).toEqual('1000px');
});
});
});

0 comments on commit 7c365cb

Please sign in to comment.