1368094 - Entering fullscreen on OSX while an update doorhanger is present causes the anchor arrow to be misaligned

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement)

Description

[Alex Thayer [:alexical] (she/her)]

This is only be relevant once Bug 1357917 lands. Something is causing the slide of the arrow attached to the anchor to be delayed in updating, so that when we move the window to the edge of the screen (for example, by entering fullscreen,) the arrow is temporarily misaligned with the hamburger button. This usually lasts for a few seconds.

Comment 1

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1368094 - Correct panel sliding on window resize

This is in response to an issue that's affecting the new app
update doorhangers on OSX, where the problem is more obvious.
On OSX, the panel styling makes it so that the doorhanger
overflows the window a little bit. This is fine until you enter
fullscreen with ctrl+command+F. At this point, the doorhanger
should come back onto the screen and the arrow should be rooted
to its anchor element (in our case the hamburger menu icon), but
instead it lags and the panel is not adjusted right away. This
is because right after the window is resized, which ends up
calling SetPopupPosition with aIsMove == false, SetPopupPosition
is called again from CheckForAnchorChange with aIsMove set to
true. There could be other solutions to this particular problem,
but since the aIsMove boolean is intended to limit the visual
noise when moving a window between screens, it seemed appropriate
for it to only prevent sliding or flipping if the panel isn't
already slid or flipped.

There was another issue affecting specifically the arrow, where
the logic for notifying observers of a positioning change in the
panel doesn't account for changes only to the position of the
anchor rect. This change adds tracking of that and sets aNotify
to true when called from ReflowFinished, since this is where
the position of the anchor element relative to the window can
need to change, even when the screen position of the panel rect
doesn't change.

Review commit: https://reviewboard.mozilla.org/r/147686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147686/

Comment 2

[Alex Thayer [:alexical] (she/her)]

Boris, I added you as a reviewer to this since Neil is out and I'm not sure who else would be best to take a look at it. If you know who might be better feel free to punt it to them. :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8876248 [details]
Bug 1368094 - Correct panel sliding on window resize

https://reviewboard.mozilla.org/r/147686/#review152026

::: layout/xul/nsMenuPopupFrame.cpp:1565
(Diff revision 1)
>    }
>  
> -  // If a panel is being moved or has flip="none", don't constrain or flip it. But always do this for
> +  nscoord oldAlignmentOffset = mAlignmentOffset;
> +
> +  // If a panel is being moved or has flip="none", don't constrain or flip it, in order to avoid
> +  // visual noise when moving windows between screens. However, if a panel is already constrained

Is it just about visual noise?  In https://bugzilla.mozilla.org/show_bug.cgi?id=536625#c0 the problem was that the panel would just end up in the wrong place altogether, no?

(Note: I've never seen this code before, so I'm not sure I can competently review it without studying it some.  But I'm also not sure who to suggest other than enn... maybe Kirk (:bytesized)?)

Comment 4

[Alex Thayer [:alexical] (she/her)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #3)
> Is it just about visual noise?  In
> https://bugzilla.mozilla.org/show_bug.cgi?id=536625#c0 the problem was that
> the panel would just end up in the wrong place altogether, no?

Hmm, I was going off of 590665, which applies the aIsMove logic to moving the panel where it was previously only applying to whether it should flip or not, but given this comment[1] on the bug you mention, I think the original reason was some kind of visual noise. I think the behavior of the popup shooting back to its original position listed in that bug were due to using "the widget's Move" instead of "the menupopup's MoveTo", and that the aIsMove logic was just an ancillary change to make dragging the popup feel less glitchy near the edge of the screen. However this is just a guess without Neil.

> (Note: I've never seen this code before, so I'm not sure I can competently
> review it without studying it some.  But I'm also not sure who to suggest
> other than enn... maybe Kirk (:bytesized)?)

Kirk, how do you feel about reviewing this, or do you happen to know of any good substitute for Neil from having worked in this file before?

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=536625#c6

Comment 5

[Robin Steuber (she/her) [:bytesized]]

I think that I am the best reviewer for it while Neil is out. I'm afraid that my experience with panels is relatively limited, but I suspect that I have more than anyone else available at the moment. I will just need some extra time to re-familiarize myself with this part of the codebase.

Comment 6

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8876248 [details]
Bug 1368094 - Correct panel sliding on window resize

https://reviewboard.mozilla.org/r/147686/#review152586

Sorry it took me so long to refamiliarize myself with this code. I've requested a few changes, but overall I think this looks good.

::: layout/xul/nsMenuPopupFrame.cpp:123
(Diff revision 1)
>    , mInContentShell(true)
>    , mIsMenuLocked(false)
>    , mMouseTransparent(false)
>    , mHFlip(false)
>    , mVFlip(false)
> +  , mIsOffset(false)

I believe I have previously experienced problems where our build automation will fail if the members in the initialization list are not in the same order as they are in the class definition. Let's move this above `mHFlip` and `mVFlip`.

::: layout/xul/nsMenuPopupFrame.cpp
(Diff revision 1)
> +  // If a panel is being moved or has flip="none", don't constrain or flip it, in order to avoid
> +  // visual noise when moving windows between screens. However, if a panel is already constrained
> +  // or flipped (mIsOffset), then we want to continue to calculate this. Also, always do this for
>    // content shells, so that the popup doesn't extend outside the containing frame.
> -  if (mInContentShell || (mFlip != FlipType_None &&
> +  if (mInContentShell || ((!aIsMove || mIsOffset) && mFlip != FlipType_None)) {
> -                          (!aIsMove || mPopupType != ePopupTypePanel))) {

Should this still check for `mPopupType != ePopupTypePanel`?

::: layout/xul/nsMenuPopupFrame.cpp:1609
(Diff revision 1)
>      // positioned at screenPoint. If not, flip the popups to the opposite side
>      // of their anchor point, or resize them as necessary.
>      bool endAligned = IsDirectionRTL() ?
>        mPopupAlignment == POPUPALIGNMENT_TOPLEFT || mPopupAlignment == POPUPALIGNMENT_BOTTOMLEFT :
>        mPopupAlignment == POPUPALIGNMENT_TOPRIGHT || mPopupAlignment == POPUPALIGNMENT_BOTTOMRIGHT;
> +    nscoord oldScreenPoint = screenPoint.x;

I'd prefer a different name for this variable. Maybe `preoffsetScreenPoint`? I assumed reading this that `oldScreenPoint` would be the coordinate of the panel prior to the `SetPopupPosition` call, which it is not.

Comment 7

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8876248 [details]
Bug 1368094 - Correct panel sliding on window resize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147686/diff/1-2/

Comment 8

[Alex Thayer [:alexical] (she/her)]

Accidentally cleared Kirk from the review list because of Review Board. Sorry about that. In any case, leaving the bz because I believe I still need a peer (layout? XPToolkit?) on this? Feel free to clear or advise as needed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8876248 [details]
Bug 1368094 - Correct panel sliding on window resize

https://reviewboard.mozilla.org/r/147686/#review154116

I totally delegated this to Kirk.  ;)  His r+ is enough here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e19a5cc45056
Correct panel sliding on window resize r=bytesized

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e19a5cc45056