XUL popups with an anchor element should hide when the anchor loses its visibility and/or frame

VERIFIED FIXED in Firefox 53

Status

()

Toolkit
XUL Widgets
VERIFIED FIXED
3 years ago
2 months ago

People

(Reporter: MattN, Assigned: Neil Deakin (mostly unavailable until September))

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

Trunk
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog ?
in-testsuite +

Firefox Tracking Flags

(firefox53+ verified, firefox54 verified)

Details

(Whiteboard: [fce-active][fxprivacy])

Attachments

(2 attachments, 6 obsolete attachments)

There are many reason ways that a popup anchor can lose its visibility and/or frame while a popup is showing. When this happens, the panel stays open in a seemingly unusual location and we currently have to workaround all of these cases in Firefox (especially for UITour). It seems like the popup code should handle this case by hiding the popup (with the usual popuphiding/popuphidden events) for the frontend.

Some cases to consider:
* Window is minimized (with @noautohide)
* Window is resized so the anchor is in the Fx overflow panel
* Window is resized so the anchor is not visible due to clipping
* Anchor element is in another popup (e.g. menu or Hello panel) and that popup hides
* Anchor element is deleted from the document
* Anchor element gets `display: none`
* Anchor element gets `visibility: hidden`
Flags: firefox-backlog?
Blocks: 969370
Neil, do you have an estimate of the feasibility of something like this?
Flags: needinfo?(enndeakin)
Essentially we would need a 'frame has changed position or visibility' notification. I think the simplest way would be to create a special type of element and frame that could be used as an anchor, as supporting this on all possible frames would be more complex and unnecessary. It would mean adding this element at all possible places the uitour popup could be anchored to though.

But a layout person might have some other ideas on the feasibility here. The visibility notification on all frames at least could be useful to fix another bug where focus is on a frame that disappears.
Flags: needinfo?(enndeakin)
I don't think we necessarily need a new notification as we already know when a popup is moving to a new position and we can check the visibility of the anchor at that point, right?
Blocks: 1091785
Blocks: 1121210
(In reply to Matthew N. [:MattN] from comment #3)
> I don't think we necessarily need a new notification as we already know when
> a popup is moving to a new position and we can check the visibility of the
> anchor at that point, right?

We need to know when the anchor moves.
See Also: → bug 1127302
Bug 1134507 also needs this bug to avoid dirty workarounds.

(In reply to Neil Deakin from comment #4)
> (In reply to Matthew N. [:MattN] from comment #3)
> > I don't think we necessarily need a new notification as we already know when
> > a popup is moving to a new position and we can check the visibility of the
> > anchor at that point, right?
> 
> We need to know when the anchor moves.

The popup already gets repositioned when that happens so I suspect some of the popup methods already get called when this happens.
> The popup already gets repositioned when that happens so I suspect some of
> the popup methods already get called when this happens.

Only if the popup happens to need to reflow when the anchor moves.
See Also: → bug 1161018
Blocks: 1207419
Bug 1004061 will make this issue more prevalent, so let's see if the new platform UI team can help with it.
Blocks: 1273644
Whiteboard: [fce-active]

Updated

a year ago
Blocks: 1225326
Whiteboard: [fce-active] → [fce-active][fxprivacy]
I think we may be able to use the upcoming 1272409 which adds a resizing observer for elements, and also notifies on visibility changes.  

We might be able to adapt this to handle visibility within popups and decks, as well as handle notifying on position changes.

Updated

11 months ago
Blocks: 1211311
Neil, could you post about your use-case on https://github.com/WICG/ResizeObserver/issues/33 ?

If we're going to broaden ResizeObserver to also support a "location observer" sort of usage (as you'd like here, I think), and if other people have similar use-cases (as it seems they might), it'd be nice to do so in an interoperable way.  And it'd be nice to rename "ResizeObserver" to something less resize-specific, before it ships & its current name is cemented into the web platform.

I think the spec editor (Aleks) is open to broadening the spec to cover this sort of thing, but motivating use-cases would help inform that decision.
Flags: needinfo?(enndeakin)
After doing some preliminary work on implementing this, it doesn't look like the ResizeObserver adds much help here. Instead just adding an object that implements nsARefreshObserver works ok with less overhead.
Flags: needinfo?(enndeakin)
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Created attachment 8811244 [details] [diff] [review]
Hide or move popups when the anchor changes

This patch moves or hides popups when the element it is anchored to changes. Only type="arrow" panels are handled by default; however followanchor="true" may be added to any popup to enable this behaviour. Popups that are opened at screen coordinates (with openPopupAtScreen) are not affected.

The popup is repositioned when the anchor's rectangle is adjusted. It handles transitions, but I don't believe this currently handles transforms that are applied, although I haven't tested this extensively.

The popup is hidden when the anchor is removed from the document, loses its frame, is collapsed, has its visibility set to hidden, or any of the anchor's ancestors are hidden or collapsed. The popup is also hidden if the anchor is in a popup which is hidden or inside a deck or tabpanel which changes to another page.

It does not currently handle when the anchor is scrolled out of view.

I confirmed that this fixes 1161018 and some issues specific to that popup, but I'm unclear about the other bug dependencies. Some I cannot reproduce. For the UItour or elsewhere it would be good to remove the workarounds to see which issues still remain.
Attachment #8811244 - Flags: feedback?(MattN+bmo)
Comment on attachment 8811244 [details] [diff] [review]
Hide or move popups when the anchor changes

Review of attachment 8811244 [details] [diff] [review]:
-----------------------------------------------------------------

The approach makes sense overall. Are you covering all of the cases mentioned in comment 0?  "It does not currently handle when the anchor is scrolled out of view." Does this also mean it doesn't cover the window resize cases I mention in comment 0? Some other cases are mentioned in associated bugs (e.g. DOM fullscreen). I'm fine fixing other cases in a follow-up if this fixes many as it seems like the right general direction.

:bytesized probably knows the popup code better than I at this point and you may also want a layout peer to review too.

:past may also be more responsive for testing the patch as it's blocking a project he's directly working on (I'm following along a bit but focused more on other projects).

popuphiding and popuphidden will still fire for popups auto-hidden by this patch, right? UITour will need that to do other cleanup.

Regarding UITour, it seems like we would want the new "followanchor" attribute on <panel id="UITourHighlightContainer" …>. #UITourTooltip is already type="arrow". Some of the  UITour.jsm code related to workarounds are [1] but that can't all be removed as some of it is necessary for other reasons so I would have to take a closer look. Other workarounds are on the webpages that use the API e.g. listening for visibilitychange in the page to handle the window being minimized.

In my testing it's definitely an improvement though I noticed that the panel may still jump to the top-left corner of the window for 1 frame after the anchor is removed and before this patch closes it. Sometimes that didn't happen though and it it's better than having it hang around that corner of the window indefinitely. I still think that that case may be detectable another way since somehow we are calculating points of 0,0 when we should be.

[1] https://dxr.mozilla.org/mozilla-central/rev/b7f895c1dc2e91530240efbf50ac063a0f8a9cb5/browser/components/uitour/UITour.jsm#1651-1652,1666-1667
[2] https://dxr.mozilla.org/mozilla-central/rev/b7f895c1dc2e91530240efbf50ac063a0f8a9cb5/browser/components/uitour/UITour.jsm#853

::: layout/xul/nsMenuPopupFrame.cpp
@@ +2468,5 @@
> +nsMenuPopupFrame::CheckForAnchorChange(nsRect& aRect)
> +{
> +  if (mAnchorType != MenuPopupAnchorType_Node || !mAnchorContent) {
> +    return false;
> +  }

I find it a bit strange that you do this check but don't include the attribute checks. Why is that? Perhaps we should change these to assertions (rather than an early return) to declare that this method should only be called when ShouldFollowAnchor would return true (like it's called in the implementation).

@@ +2472,5 @@
> +  }
> +
> +  // If automatic positioning is turned off, return early. This is checked here so
> +  // that positioning can be disabled while the popup is open.
> +  if (!mShouldAutoPosition) {

Should mShouldAutoPosition also be checked in ShouldFollowAnchor?

::: layout/xul/nsXULPopupManager.cpp
@@ +414,5 @@
>  void
> +nsXULPopupManager::WillRefresh(mozilla::TimeStamp aTime)
> +{
> +  for (int32_t i = 0; i < 2; i++) {
> +    nsMenuChainItem* item = i == 0 ? mPopups : mNoHidePanels;

Can't this be refactored into a helper function which gets called twice (once for mPopups and once for mNoHidePanels) instead of this harder to read loop idea?
Attachment #8811244 - Flags: feedback?(MattN+bmo) → feedback+
Created attachment 8812679 [details]
Video of patched #UITourHighlightContainer jumping to top-left of the window upon anchor removal

This is a video capturing a flicker (one frame) with the panel at the top-left corner after removing the anchor node with attachment 8811244 [details] [diff] [review] applied. Previously the panel would remain at this offset indefinitely so the new behaviour is definitely better.

--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -258,4 +258,5 @@
     <!-- type="default" forces frames to be created so that the panel's size can be determined -->
     <panel id="UITourHighlightContainer"
+           followanchor="true"
            type="default"
            hidden="true"
> "It does not currently handle when the anchor is
> scrolled out of view." Does this also mean it doesn't cover the window
> resize cases I mention in comment 0?

I meant that it doesn't currently handle the case where the anchor is a scrolling area, and the scroll position changes to move it so that it isn't visible.

But it also won't handle the case where the window is resized and the anchor is clipped off.

The window being minimized is a different bug.


> popuphiding and popuphidden will still fire for popups auto-hidden by this
> patch, right? UITour will need that to do other cleanup.

Yes.

> In my testing it's definitely an improvement though I noticed that the panel
> may still jump to the top-left corner of the window for 1 frame after the
> anchor is removed and before this patch closes it. Sometimes that didn't
> happen though and it it's better than having it hang around that corner of
> the window indefinitely. I still think that that case may be detectable
> another way since somehow we are calculating points of 0,0 when we should be.

I see the anchor being collapsed or hidden and then immediately being assigned another size. I don't see the uitour highlight flash in the corner however. 


> I find it a bit strange that you do this check but don't include the
> attribute checks. Why is that? Perhaps we should change these to assertions
> (rather than an early return) to declare that this method should only be
> called when ShouldFollowAnchor would return true (like it's called in the
> implementation).

I hadn't planned on implementing the ability to change the attribute or modify the follow state while the popup was open. But I think it might be clearer to just allow that.


> Can't this be refactored into a helper function which gets called twice
> (once for mPopups and once for mNoHidePanels) instead of this harder to read
> loop idea?

I'm instead going to remove the two lists in bug 1318705.
I tested the patch with the new permission doorhangers that are now in Nightly and it seems to work fine. Unfortunately it doesn't fix the case when the window is being minimized as you mention, which I expect is the most likely one for users to hit.

--- a/browser/base/content/popup-notifications.inc
+++ b/browser/base/content/popup-notifications.inc
@@ -2,6 +2,7 @@
 
     <panel id="notification-popup"
            type="arrow"
+           followanchor="true"
            position="after_start"
            hidden="true"
            orient="vertical"
With simple panels I only see an issue on Mac where the popup isn't hiding when being minimized.

The notification panels however are being created with noautohide equal to false, then noautohide is being set to true, and then the popup is being opened.

On Linux, noautohide="false" panels are created as GTK popup widgets which won't react to the parent window being minimized. noautohide="true" panels are created as GTK top level windows which will do this. You cannot change the widget type after the widget has been created, so changing the noautohide to true won't affect the minimizing behaviour.

The options are:
  1. Set the noautohide attribute to true by default. I'm not clear what it's being changed for though.
  2. If you need to have noautohide="false" on the same popup, you might consider just creating two different popups.
  3. Remove the panel such that its nsIFrame and widget are destroyed, then create it again.
  4. Listen for the sizemodechanged event and hide/show the popup
  5. It's possible some if some change is needed for the Mac, it could be applied to Linux.
I filed bug 1320341 and a patch on the Mac minimizing issue.
This is very informative, thanks Neil. The reason for changing noautohide after creation is that most PopupNotifications.show() consumers were modified to be persistent, but not all. We didn't want to affect add-on consumers in particular, the plan was for the change to be backwards compatible.

It sounds like this would work then: swap the logic and make noautohide="true" the default on creation and only remove the attribute before opening the popup. This way persistent popups will be properly minimized and old-style, non-persistent ones will continue to have the same issue on minimize (i.e. no change). Does this sound reasonable?
That sounds like it would work.
Filed bug 1320361.
(In reply to Neil Deakin from comment #16)
> The notification panels however are being created with noautohide equal to
> false, then noautohide is being set to true, and then the popup is being
> opened.
> 
> On Linux, noautohide="false" panels are created as GTK popup widgets which
> won't react to the parent window being minimized. noautohide="true" panels
> are created as GTK top level windows which will do this. You cannot change
> the widget type after the widget has been created, so changing the
> noautohide to true won't affect the minimizing behaviour.
> …
>   3. Remove the panel such that its nsIFrame and widget are destroyed, then
> create it again.
> …

This is what we do in UITour.jsm for related issues but with hidden=true (my understanding is that it's sufficient to destroy the frame): https://dxr.mozilla.org/mozilla-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/browser/components/uitour/UITour.jsm#1788-1800
Created attachment 8814951 [details] [diff] [review]
Hide or move popups when the anchor changes, v2

This version makes each popup be a listener for changes, which properly works when multiple windows are open.

I also added some visibility checks for when the anchor is scrolled to be clipped out of view; this also handles when the window is resized such that the anchor is no longer visible. When either situation happens, the popup hides. I'm not sure what the desire is here though -- do we want popups to disappear in all of these cases?
Attachment #8811244 - Attachment is obsolete: true
Created attachment 8819378 [details] [diff] [review]
Hide or move popups when the anchor changes, v3

I went back to not handling scrolled-out-of-view. That could be added later.

This patch also fixes crashing due to references being deleted while manipulating the popups.
Created attachment 8819968 [details] [diff] [review]
Hide or move popups when the anchor changes, v4

Minor polish over previous patch.

Two notes:

1. I'm assuming that the use of Flush_Layout in the call to AddRefreshObserver is the right one to use.
2. I don't think this handles transforms; is it just a matter of calling nsLayoutUtils::TransformFrameRectToAncestor?
Attachment #8814951 - Attachment is obsolete: true
Attachment #8819378 - Attachment is obsolete: true
Attachment #8819968 - Flags: review?(tnikkel)
Comment on attachment 8819968 [details] [diff] [review]
Hide or move popups when the anchor changes, v4

A couple of general comments.

anchor->GetRect() is the rect of the frame relative to its parent frame. So if the anchor frame doesn't move relative to it's parent, but an ancestor does move then the anchor has effectively moved, but GetRect will return the same result. Using TransformFrameRectToAncestor would fix this, if you choose the ancestor as the root most frame.

nsARefreshObserver isn't quite what you want: the name WillRefresh says why, it's called before we refresh, ie before we flush layout. There is nsAPostRefreshObserver but that's not right either as it gets called after painting, which is too late.

I think we'll need something like this block

https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/layout/base/nsRefreshDriver.cpp#1889

The main points are that it gets called after flushing style and layout, but only if there was some change(s) to flush, and it gets called before painting.

Since this call happens on every tick of the refresh driver we want to be careful not to do too much.

It's not clear (on a quick reading) to me if we only register with the refresh driver when the popup is open and showing. We should only register if the popup is open.
Attachment #8819968 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #25)
> It's not clear (on a quick reading) to me if we only register with the
> refresh driver when the popup is open and showing. We should only register
> if the popup is open.

Yes, only while the popup is open.
Duplicate of this bug: 1332864
Created attachment 8830374 [details] [diff] [review]
Hide or move popups when the anchor changes, v4

This version the same computed anchor rectangle that SetPopupPosition uses to account for transforms and when the anchor is in a different document.

It also switches to just do what is needed within Tick() instead of using a refresh observer.

I spent quite a bit of time on investigating the two popup notification tests that now have issues. One issue with these tests is that they dismiss a notification without waiting for it to hide then carry on with the next part of the test. The tests also listen for various events but use executeSoons so various callbacks happen before/after notifications. It was too messy to debug or fix, so i just disable the following for these tests.
Attachment #8830374 - Flags: review?(tnikkel)
Comment on attachment 8830374 [details] [diff] [review]
Hide or move popups when the anchor changes, v4

>diff --git a/browser/base/content/test/popupNotifications/head.js b/browser/base/content/test/popupNotifications/head.js

I don't understand these tests, someone who does should review if this is okay.


>diff --git a/layout/base/nsRefreshDriver.cpp b/layout/base/nsRefreshDriver.cpp

> 
>+#ifdef MOZ_XUL
>+  nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
>+  if (pm) {
>+    pm->UpdatePopupPositions();
>+  }
>+#endif

Can we have a local bool that gets set to true in the two places that mNeedToRecomputeVisibility gets set to true in ::Tick() and make this call conditional on that bool?


>diff --git a/layout/xul/nsMenuPopupFrame.cpp b/layout/xul/nsMenuPopupFrame.cpp
>+nsRect
>+nsMenuPopupFrame::ComputeAnchorRect(nsPresContext* aRootPresContext, nsIFrame* aAnchorFrame)
>+{
>+  // Get the root frame for a reference
>+  nsIFrame* rootFrame = aRootPresContext->FrameManager()->GetRootFrame();
>+
>+  // The dimensions of the anchor
>+  nsRect parentRect = aAnchorFrame->GetRectRelativeToSelf();

I realize you are just moving code, but I don't know why this is called parentRect. It doesn't seem to have anything to do with the parent, as the function call says it is relative to the frame itself.

>diff --git a/layout/xul/nsMenuPopupFrame.h b/layout/xul/nsMenuPopupFrame.h
>+  // Given an anchor frame, compute the anchor rectangle relative to the popup
>+  // frame, using the popup frame's app units, and taking into account transforms.
>+  nsRect ComputeAnchorRect(nsPresContext* aRootPresContext, nsIFrame* aAnchorFrame);

Isn't it relative to the screen? Not the popup frame?

>diff --git a/layout/xul/nsXULPopupManager.h b/layout/xul/nsXULPopupManager.h
>+  // The last seen position of the anchor.
>+  nsRect mCurrentRect;

Can you mention what this is relative to in the comment?

>-  // removes an item from the chain. The root pointer must be supplied in case
>+  // Removes an item from the chain. The root pointer must be supplied in case
>   // the item is the first item in the chain in which case the pointer will be
>   // set to the next item, or null if there isn't another item. After detaching,
>-  // this item will not have a parent or a child.
>+  // this item will not have a parent or a child.  
>   void Detach(nsMenuChainItem** aRoot);

You're adding some trailing space here.

Otherwise looks good.
Attachment #8830374 - Flags: review?(tnikkel)
Attachment #8819968 - Attachment is obsolete: true
So nsRefreshDriver::Tick() is called for each document, whereas adjusting popups should only happen once (there is only ever one nsXULPopupManager).

I can easily pass the nsPresContext to nsXULPopupManager::UpdatePopupPositions and update only those popups that match. Does that seen reasonable, or is there another way?
Flags: needinfo?(tnikkel)
Ahh! Good catch.

(In reply to Neil Deakin from comment #30)
> So nsRefreshDriver::Tick() is called for each document

Not quite, each content tab gets its own refresh driver, but any iframes in that tab will share the same refresh drver.

So how about we only call UpdatePopupPositions if mPresContext IsRoot() and IsChrome()? I think that should do what we want. We could make it conditional on XRE_IsParentProcess() as well if we wanted.
Flags: needinfo?(tnikkel)
But that will still get called multiple times when multiple windows are open right?
Flags: needinfo?(tnikkel)
Oh right.

If you wanted to go the route of passing a prescontext to UpdatePopupPositions to limit which popups to update then you will have to also handle popups in any child prescontext that use the same refresh driver (not just any child prescontext).

The other option would be to add a function NeedUpdatePopupPositions on the refresh driver, and call that whenever a popup is opened, and a function clear that state when all popups are closed.
Flags: needinfo?(tnikkel)
Created attachment 8835488 [details] [diff] [review]
Hide or move popups when the anchor changes, v5
Attachment #8830374 - Attachment is obsolete: true
Attachment #8835488 - Flags: review?(tnikkel)
Comment on attachment 8835488 [details] [diff] [review]
Hide or move popups when the anchor changes, v5

>+  if (mNeedToRecomputeVisibility) {
>+    // Recompute approximate frame visibility if it's necessary and enough time
>+    // has passed since the last time we did it.
>+    if (!mThrottled &&
>+        aNowTime >= mNextRecomputeVisibilityTick &&
>+        !presShell->IsPaintingSuppressed()) {
>+      mNextRecomputeVisibilityTick = aNowTime + mMinRecomputeVisibilityInterval;
>+      mNeedToRecomputeVisibility = false;
> 
>-    presShell->ScheduleApproximateFrameVisibilityUpdateNow();
>+      presShell->ScheduleApproximateFrameVisibilityUpdateNow();
>+    }
>+
>+#ifdef MOZ_XUL
>+    // Update any popups that may need to be moved or hidden due to their
>+    // anchor changing.
>+    nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
>+    if (pm) {
>+      pm->UpdatePopupPositions(this);
>+    }
>+#endif
>   }

Unfortunately mNeedToRecomputeVisibility is set to true in other places then just the two places in ::Tick(). So I think we need our own bool to track this.
Attachment #8835488 - Flags: review?(tnikkel)

Comment 36

6 months ago
Comment on attachment 8835488 [details] [diff] [review]
Hide or move popups when the anchor changes, v5

(In reply to Timothy Nikkel (:tnikkel) from comment #29)
> I don't understand these tests, someone who does should review if this is okay.

This is not okay, unfortunately. The tests shouldn't change the behavior of the panel, otherwise we're not testing what we ship and we may likely break things.

We should make sure that all these tests work with the new behavior, however we can do that in a separate bug. Just set followanchor="false" in production code for the moment, and keep the tests unchanged.
Attachment #8835488 - Flags: feedback-
(In reply to Timothy Nikkel (:tnikkel) from comment #29)
> 
> Can we have a local bool that gets set to true in the two places that
> mNeedToRecomputeVisibility gets set to true in ::Tick() and make this call
> conditional on that bool?
> 

Actually this isn't going to work exactly. There are two cases where a popup will need to be updated when a restyle or relayout does not occur: when a parent popup of the anchor is closed and when a parent deckframe on the anchor changes its current index. Possibly there could be others.

The anchor could be in a different document than the popup, so setting a flag in the refresh driver may be complicated. Perhaps checking every tick is easiest (but almost no code will run when no popup is open).
Relman was asked to track some of these issues for 53, in bug 1332864. 
I'm not sure if this scope of change is feasible to bring to 52, but if there are elements that make sense to uplift, we could still take patches for 53 aurora.
(In reply to Neil Deakin from comment #37)
> (In reply to Timothy Nikkel (:tnikkel) from comment #29)
> > 
> > Can we have a local bool that gets set to true in the two places that
> > mNeedToRecomputeVisibility gets set to true in ::Tick() and make this call
> > conditional on that bool?
> > 
> 
> Actually this isn't going to work exactly. There are two cases where a popup
> will need to be updated when a restyle or relayout does not occur: when a
> parent popup of the anchor is closed and when a parent deckframe on the
> anchor changes its current index. Possibly there could be others.
> 
> The anchor could be in a different document than the popup, so setting a
> flag in the refresh driver may be complicated. Perhaps checking every tick
> is easiest (but almost no code will run when no popup is open).

Okay, lets just always check.
Created attachment 8837138 [details] [diff] [review]
Hide or move popups when the anchor changes, v6
Attachment #8835488 - Attachment is obsolete: true
Attachment #8837138 - Flags: review?(tnikkel)
Attachment #8837138 - Flags: review?(tnikkel) → review+

Comment 41

6 months ago
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d280a7a149bb
panels should watch their anchors for position and visibility changes and update accordingly, r=tn

Comment 42

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d280a7a149bb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

6 months ago
Blocks: 1340538

Comment 43

6 months ago
[Tracking Requested - why for this release]:
Dependency for bug 1340538, which is about the notification panel not closing properly on page refresh, but moving to a different position on the screen.

Neil, how do you feel about uplifting this to early Beta?
status-firefox53: --- → affected
tracking-firefox53: --- → ?
Flags: needinfo?(enndeakin)
Might be best to leave this in aurora and do some testing around popup behavior. 
Are we actively using UITours any more?
tracking-firefox53: ? → +
status-firefox53: affected → fix-optional
If you do want to uplift, can you explain more how we should test? Thanks.
I think there could be issues that come up, but they are easy to workaround by adding followanchor="false" to the popup as needed.
Flags: needinfo?(enndeakin)

Comment 47

5 months ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #45)
> If you do want to uplift, can you explain more how we should test? Thanks.

I'll add this to the uplift request.

Comment 48

5 months ago
Comment on attachment 8837138 [details] [diff] [review]
Hide or move popups when the anchor changes, v6

Approval Request Comment
[Feature/Bug causing the regression]: Permission Notifications
[User impact if declined]: See bug 1340538, that depends on this one
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No, but no regressions were noticed since this landed
[Needs manual test from QE? If yes, steps to reproduce]: For this bug, we just need exploratory testing around arrow panel behavior. Try to do something that causes the panel's anchor to move or disappear, and check that no regressions occur when this patch is present.
[List of other uplifts needed for the feature/fix]: This is for bug 1340538
[Is the change risky?]: Yes, but there is a known mitigation strategy
[Why is the change risky/not risky?]: Per comment 46, if issues come up with some popups, they can be worked around by adding followanchor="false" to the popup as needed.
[String changes made/needed]: None
Attachment #8837138 - Flags: approval-mozilla-beta?
Comment on attachment 8837138 [details] [diff] [review]
Hide or move popups when the anchor changes, v6

This does seem a little risky but it includes new test coverage.
Let's go ahead with it for 53 beta 2.
Attachment #8837138 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Paolo, for testing, can you give some other example pages where this is used? 

Andrei, can your team take a look at some exploratory testing around this issue? STR are here: https://bugzilla.mozilla.org/show_bug.cgi?id=1340538#c0.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(andrei.vaida)
This is hitting conflicts in layout/base/nsRefreshDriver.cpp, can we get a rebased patch?

Comment 52

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2f9fc3b858d7
status-firefox53: fix-optional → fixed
Flags: in-testsuite+
I followed the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1340538#c0 and executed some additional exploratory testing. I can confirm this issue is fixed as no other bugs were found.

I verified using Fx 54.0a2, build ID: 20170316004004, and Fx 53.0b2, build id: 20170313154936, on Windows 10 x64, Mac OS X 10.12.3 and Ubuntu 14.04 LTS.

Cheers!
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: needinfo?(andrei.vaida)
Depends on: 1340413

Updated

5 months ago
Flags: needinfo?(paolo.mozmail)

Updated

4 months ago
Depends on: 1349094

Updated

4 months ago
Depends on: 1358487

Updated

4 months ago
Depends on: 1358713

Updated

2 months ago
Blocks: 1369488
You need to log in before you can comment on or make changes to this bug.