Closed Bug 1368094 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Application Update, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: dthayer, Assigned: dthayer)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
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 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)?)
(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
Flags: needinfo?(ksteuber)
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.
Flags: needinfo?(ksteuber)
Attachment #8876248 - Flags: review?(bzbarsky) → review?(ksteuber)
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.
Attachment #8876248 - Flags: review?(ksteuber) → review+
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 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.
Attachment #8876248 - Flags: review?(bzbarsky)
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e19a5cc45056
Correct panel sliding on window resize r=bytesized
https://hg.mozilla.org/mozilla-central/rev/e19a5cc45056
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.