New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popover: Refactor popover to clarify computations #6799

Merged
merged 11 commits into from May 23, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented May 17, 2018

and support setting max-width/max-height depending on the available space

This will fix the issues we have where popovers are cut over the edges of the browser window by adapting the height/width of the popovers.

Still having trouble with a z-index issue at the moment.

closes #4519, #6026, #6026, #4949, #5276, #6504,

if ( event.type === 'scroll' && this.nodes.content.contains( event.target ) ) {
return;
}
this.rafHandle = window.requestAnimationFrame( () => this.computePopoverPosition() );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 17, 2018

Member

Minor: maybe we should save the id returned by requestAnimationFrame and invoke window.cancelAnimationFrame() when unMounting if we have a valid id to cancel.

This comment has been minimized.

@youknowriad

youknowriad May 17, 2018

Contributor

It's already the case I believe

forcedXAxis || xAxis,
];
if (
! this.state.popoverLeft ||

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 17, 2018

Member

I'm not understanding why we have the condition ! this.state.popoverLeftshouldn't this case be covered by this.state.popoverLeft !== popoverLeft? I think I'm missing something.

This comment has been minimized.

@youknowriad

youknowriad May 17, 2018

Contributor

Yes, it's a leftover :)

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 17, 2018

Automatic test cases need an update, but manually testing things are looking good and I did not notice any problem in my initial tests :)

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 17, 2018

I fixed the tests, extracted the computing to an utility to ease unit-testing. I'm adding a design feedback. I think this improves things above popovers. It's not solving everything but it's a good improvement I think.

youknowriad added some commits May 21, 2018

this.toggleWindowEvents( true );
this.focus();
setTimeout( () => this.focus() );

This comment has been minimized.

@youknowriad

youknowriad May 22, 2018

Contributor

I'd love to get rid of this timeout but for some mysterious reason, it's not working properly without it.

  • Try removing the timeout
  • Open the inserter
  • The search input is not focused.

I tried debugging and for some reason the call to input.focus() is not focusing the input (console logging the document.actiiveElement.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 22, 2018

Member

I tried to debug, and I did not find a good solution to this, It looks like although the parent component was mounted the child is not right away in a state that we can focus, this discussion seems related https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example.

What complicates, even more, the things is that if we use withSafeTimeout, we create the z-index bug ( i tested it again).

Maybe we can move the setTimeout inside the focus function so inside the timeout we don't reference this, props, etc.. we reference the dom node something like:

		const focusNode = ( domNode ) => setTimeout( () => domNode.focus() );
		const domNode = this.contentNode.current;
		// Find first tabbable node within content and shift focus, falling
		// back to the popover panel itself.
		const firstTabbable = focus.tabbable.find( domNode )[ 0 ];
		focusNode( firstTabbable ? firstTabbable : domNode );

This comment has been minimized.

@youknowriad

youknowriad May 22, 2018

Contributor

I don't expect this to create any issue regarding "this" because it's during mounting but even though, I like your proposal let's move it to focus and add a comment.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 23, 2018

Member

Maybe we can also add a comment specifying why we are using a set timeout, and why withSafeTimeout was not possible to use.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented May 22, 2018

I think we get this in as it solves a lot of issues. thoughts @jorgefilipecosta

@@ -794,7 +794,7 @@ export class RichText extends Component {
changeFormats( formats ) {
forEach( formats, ( formatValue, format ) => {
if ( format === 'link' ) {
if ( formatValue !== undefined ) {
if ( !! formatValue ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 22, 2018

Member

I'm not understanding the relation of this changes in RichText with the changes in popover. Is it because of the link component? Without this changes we cause an unexpected bahaviour or they are just code improvement?

This comment has been minimized.

@youknowriad

youknowriad May 22, 2018

Contributor

The relation is indirect. There's a bug in master where if you click "Escape" on a newly created link (empty), the modal is not closed. This change ensures the modal is closed when you do so.

The reason it's in this PR is that the popover change makes the e2e test fail while it's not failing in master (false positive). I'm not certain I understand exactly why it's not failing in master tests because, in my manual testing, it's not working properly.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 23, 2018

Member

Thank you for the clarification, e2e tests on our CI sometimes pass when they should fail. For example in #6877 I added a test case that should fail because the fix was not yet merged, running tests in my computer the test fails all the time, but in our CI it is reported as passing, not certain why.

expect( computePopoverPosition( anchorRect, contentSize, 'top right' ) ).toEqual( {
contentHeight: null,
contentWidth: null,
isMobile: false,

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 22, 2018

Member

All the test cases test the non-mobile case should we add a mobile test case?

This comment has been minimized.

@youknowriad

youknowriad May 23, 2018

Contributor

Yep, good catch it was less important but I'll see if I can tweak the window size.

};
const bottomAlignment = {
popoverTop: anchorRect.bottom,
contentHeight: anchorRect.bottom + HEIGHT_OFFSET + height > window.innerHeight ? window.innerHeight - HEIGHT_OFFSET - anchorRect.bottom : height,

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 22, 2018

Member

Maybe window.innerHeight, and window.innerWidth can be parameters with default values to the window properties. I think it may simplify the addition of test cases in the future.

// Choosing the x axis
let chosenXAxis;
let contentWidth = null;
if ( xAxis === 'center' && centerAlignment.contentWidth === width ) {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 22, 2018

Member

Minor: Maybe we can use a switch.

On the switches we would do:

case 'center':
	chosenXAxis = centerAlignment.contentWidth === width ? 'center' : undefined;

After the switch, we would check if chosenXAxis was undefined if yes we would use our last else. It is just a personal preference normally I prefer switches instead of a series of else.

It may also make sense to extract some parts of the function into smaller ones e.g: one part that deals with the x and other that deals with y, just to make computePopoverPosition smaller :)

This comment has been minimized.

@youknowriad

youknowriad May 23, 2018

Contributor

I don't like the switch in this case personally, I like "dumb" code :P but I like the splitting proposal which I did.

@jorgefilipecosta

Excellent work on this refactor. Testing the popover it looks like a big improvement. 👍
There is some debugging that could be made like understand why withSafeTimeout interferes with z-index if time allows in the future. But given the number of issues closed and the noticeable improvements for the user that we have I feel we should get it in soon so we can test it more before the next release.

@youknowriad youknowriad merged commit c8f8806 into master May 23, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.09% but relative coverage increased by +43.42% compared to cd5779d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/popover-maxwidth-height branch May 23, 2018

@mtias mtias added this to the 3.0 milestone Jun 4, 2018

return;
}
// Without the setTimeout, the dom node is not being focused
// Related https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example
const focusNode = ( domNode ) => setTimeout( () => domNode.focus() );

This comment has been minimized.

@aduth

aduth Jun 25, 2018

Member

Where are we canceling this timeout if the component is unmounted? Why aren't we using withSafeTimeout ?

This comment has been minimized.

@youknowriad

youknowriad Jun 25, 2018

Contributor

I did use withSafeTimeout originally but It breaks the design in a super weird way. (see 66b3444) The usage of the higher order component even if it's not doing anything to the DOM was creating some weird z-index issues, especially visible on mobile.

This comment has been minimized.

@aduth

aduth Jul 9, 2018

Member

What about:

Where are we canceling this timeout if the component is unmounted?

This comment has been minimized.

@youknowriad

youknowriad Nov 5, 2018

Contributor

This is already done in master

return;
}
// Without the setTimeout, the dom node is not being focused

This comment has been minimized.

@aduth

aduth Jul 30, 2018

Member

This is treating the symptom, not the cause, and the comment reads as such.

The real cause is a combination of:

  • When initially mounted, the popover doesn't have a Slot context in which to render. When this becomes available, it re-renders the content into the slot (an unmount and remount operation).
  • When initially mounted, the container has a style of visibility: hidden, so focusing would not select the container.

This comment has been minimized.

@aduth

aduth Jul 30, 2018

Member

See also: #8218

@@ -100,27 +96,22 @@ class Popover extends Component {
return;
}
const { content } = this.nodes;
if ( ! content ) {
if ( ! this.contentNode.current ) {

This comment has been minimized.

@aduth

aduth Jul 30, 2018

Member

Related to earlier comment, we shouldn't need to check this here. If the component causes Popover#focus to be called during its componentDidMount, we should be able to assume that the ref.current is assigned.

}
throttledComputePopoverPosition( event ) {
if ( event.type === 'scroll' && this.contentNode.current.contains( event.target ) ) {

This comment has been minimized.

@aduth

aduth Nov 2, 2018

Member

It took me more than a minute to grasp why this condition is here. It seems obvious in retrospect, but a comment would have been nice.

@aduth aduth referenced this pull request Nov 2, 2018

Open

Components: Improve Popover computations #11430

1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment