Closed Bug 1400259 Opened 7 years ago Closed 7 years ago

Last used submenu/item is visible/highlighted in "Hamburger" (and page action, and other photon panel) menu(s) after landing patch from bug #1374749

Categories

(Firefox :: Toolbars and Customization, defect, P1)

Product:
Firefox
Component:
Toolbars and Customization
Version:
57 Branch
Platform:
x86_64
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + verified
firefox58 + verified

People

(Reporter: Virtual, Assigned: mstange)

References

Details

(Keywords: nightly-community, regression, reproducible, Whiteboard: [reserve-photon-structure])

Attachments

(4 files, 1 obsolete file)

screencast.mp4
296.44 KB, video/mp4
Details
flicker.gif
256.54 KB, image/gif
Details
Bug 1400259 - Extend the workaround to cover all popup states and both opacity and transform.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
combined patch for 57
2.06 KB, patch
ritu
: approval-mozilla-beta+
ritu
: approval-mozilla-release+
Details | Diff | Splinter Review
Attached video screencast.mp4 — 
STR:
1. Open "Hamburger" menu
2. Press "Help" item
3. Press "Esc" keyboard button
4. Open "Hamburger" menu
and see that last used "Help" menu is visible, instead of "Hamburger" menu.
When "Add-ons"/"Options"/"Customize..." items are used, they're just highlighted.
It shows proper menu after moving mouse on some menu items.

Regression caused by:
Bug #1374749

Regression pushlog:
https://hg.mozilla.org/integration/autoland/rev/5b700335fc30f2faf8aa12c9e38bc05e6c1e5e22
Flags: needinfo?(mdeboer)
Whiteboard: [photon] [triage] → [photon-structure] [triage]
[Tracking Requested - why for this release]:
Visible UI regression.
tracking-firefox57: --- → ?
Component: Menus → Toolbars and Customization
Priority: -- → P3
Whiteboard: [photon-structure] [triage] → [reserve-photon-structure]
Flags: qe-verify?
Summary: Last used submenu/item is visible/highlighted in "Hamburger" menu caused by patch from bug #1374749 → Last used submenu/item is visible/highlighted in "Hamburger" menu after landing patch from bug #1374749
Flags: needinfo?(gijskruitbosch+bugs)
So uh, I don't like this much, but I *think* this might be a layout/graphics bug.

I tried a variety of things and couldn't really get this bug to go away.

As a "surely this must do it" option, I applied the following patch:

https://pastebin.mozilla.org/9032765

diff --git a/browser/components/customizableui/PanelMultiView.jsm b/browser/components/customizableui/PanelMultiView.jsm

           for (let panelView of this._viewStack.children) {
             if (panelView.nodeName != "children") {
               panelView.__lastKnownBoundingRect = null;
               panelView.style.removeProperty("min-width");
               panelView.style.removeProperty("max-width");
+              if (panelView != this._mainView) {
+                Cu.reportError("Display none'ing " + panelView.id);
+                panelView.style.display = "none";
+              }
             }
           }

This sets the non-main views to display: none when the popuphidden event is received, and it logs a message saying this has happened. I can verify that this is being applied to the view opened when following the steps in comment #0, but *somehow* it still flashes up on my Windows machine. If I try to reopen the view, it doesn't work because display: none has kicked in.

How is this possible?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dholbert)
*something something layers*, probably.  mstange has worked closer to layers code, so I humbly punt you to him.

(Also, for the record -- I can't reproduce on my Linux desktop, but I can reproduce the flicker that Gijs describes on the Acer quantum reference hardware.  Not sure if there's a platform dependency here, or if my linux hardware is just fast enough to make the flicker imperceptible.)
Flags: needinfo?(dholbert) → needinfo?(mstange)
More direct STR, from Gijs and e.g. dupe bug 1401182:
 1. Click hamburger menu
 2. click "More"
 3. hit escape
 4. wait a second or two
 5. Click hamburger menu

Actual Results: the "More" panel is briefly visible (as the menu-open animation is playing, I think?)

(Note that these don't match the STR in comment 0, but I'm told it's a simpler version of the same bug.)
Blocks: 1401383
Summary: Last used submenu/item is visible/highlighted in "Hamburger" menu after landing patch from bug #1374749 → Last used submenu/item is visible/highlighted in "Hamburger" (and page action, and other photon panel) menu(s) after landing patch from bug #1374749
Perhaps mattwoodrow has an idea of what's going on here?
Flags: needinfo?(matt.woodrow)
Flags: qe-verify? → qe-verify+
Hi Gijs, I know the team has triaged this as a P3. Does that mean it will get fixed in 57? I hope it does. I tried the STR and this bug is kinda sloppy :(
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Ritu Kothari (:ritu) from comment #13)
> Hi Gijs, I know the team has triaged this as a P3. Does that mean it will
> get fixed in 57?

Maybe. In any case we don't triage higher than P3 at this point. Marco Mucci can tell you more.

> I hope it does. I tried the STR and this bug is kinda
> sloppy :(

Yes, but I'm also fairly sure this is a layout/graphics issue, and I've already needinfo'd half the world. Not sure how else to get more traction here.

Anyway, bug 1401991 might help here.
Flags: needinfo?(gijskruitbosch+bugs)
In addition to the STR provided, simply clicking anywhere outside the menu area so that the menu goes away also causes this to happen.  Don't need to use the ESC key.

Also, any selection of a second level menu item causes this issue to appear.  For example, selecting any action under the Help menu means that the Help menu will be shown on top the next time you open the hamburger menu.  However, third level menus do *not* show this.  For example:

[Library > select Screenshots] *will* cause the Library submenu to be shown the next time the hamburger menu is opened.

[Library > History > select View History Sidebar] will *not* cause the History submenu to be shown.

Same thing with not selecting a menu item.

[Library > cancel menu] *will* cause the Library submenu to be shown the next time the hamburger menu is opened.

[Library > History > cancel menu] will *not* cause the History submenu to be shown.

Since Gijs noted a bit of code he was testing with a stack, this might be an issue with an incorrect handling of clearing the stack?  Maybe an incorrect comparison for how much stuff is in the stack before deciding to clear it? (EG: requiring >1 on a 0-based count to trigger a clear, instead of >0 ?)
In my testing, I can't reproduce anymore with a build including bug 1401991.
Blocks: 1401991
Thanks Gijs. Appreciate your due diligence. Will try to get the fix from that bug uplifted to Beta57 in a few days.
Sounds like this might only affect Windows, glad you've found a workaround though.

Do we make all changes to the popup window while it was hidden (and when repaint requests would be suppressed)?

It sounds like making the window visible again isn't triggering us to do a repaint (through a bug in the widget/ code), so we end up displaying the old contents until something else triggers a paint in that window.
Flags: needinfo?(matt.woodrow)
(In reply to :Gijs from comment #16)
> In my testing, I can't reproduce anymore with a build including bug 1401991.

So, false alarm, I can reproduce on Windows 10 with yesterday's nightly. :-(

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Sounds like this might only affect Windows, glad you've found a workaround
> though.

Sadly, it seems I was mistaken.

> Do we make all changes to the popup window while it was hidden (and when
> repaint requests would be suppressed)?

Not all changes, but some of them, yes.

> It sounds like making the window visible again isn't triggering us to do a
> repaint (through a bug in the widget/ code), so we end up displaying the old
> contents until something else triggers a paint in that window.

Is there a way to debug this so we can get it fixed in layers / painting code, or to work around it from the frontend code? I'm happy to help gather info here, but I don't really know where to start.
Flags: needinfo?(matt.woodrow)
Has Regression Range: --- → yes
Has STR: --- → yes
Can you check whether you can still reproduce this in the latest Nightly? Thanks!
Flags: needinfo?(mdeboer) → needinfo?(Virtual)
(In reply to Mike de Boer [:mikedeboer] from comment #20)
> Can you check whether you can still reproduce this in the latest Nightly?
> Thanks!

Yes, I can still reproduce issue in latest Mozilla Firefox Nightly 58.0a1 (2017-10-02) (64-bit) on Windows 7 (64-bit).
Flags: needinfo?(Virtual)
Are you sure this is a correct duplicate? This bug doesn't seem to match my reported bug, neither the description nor the attached video. Also bug 1404900 renders correctly on smaller screens where the menus really require scrollbars. 
I'll try a screen recording again tomorrow.
(In reply to TMart from comment #23)
> Are you sure this is a correct duplicate? This bug doesn't seem to match my
> reported bug, neither the description nor the attached video. Also bug
> 1404900 renders correctly on smaller screens where the menus really require
> scrollbars. 
> I'll try a screen recording again tomorrow.

It wasn't the correct dupe, I noticed this morning that this had happened, and re-marked it as a duplicate of bug 940733 instead, which I do think is correct, but feel free to ping me on that bug if you think there's a difference with that bug, too.
That seems to be correct, thank you. Bugzilla was very slow for me today, didn't get a notification for your re-mark.
Noting this likely won't get a fix for 57 release.  (It is P3 and unassigned)
Because of getting several duplicate reports, we may want to take another look. 

jimm, what do you think if you glance over the duplicates? It may just be more reasonable to investigate this for 58.
tracking-firefox58: ? → +
Flags: needinfo?(jmathies)
or maybe we should backout patch which caused this regression, as it only added new animation
I'd like to add that this bug also seems fixed for me (tested on 57.0b6), similar to Gij's experience in comment c16.
I can still reproduce this issue with STR posted in comment #0 (see also "screencast.mp4" in Attachments to see how it looks like) in latest Mozilla Firefox Nightly 58.0a1 (2017-10-12).
(In reply to Ritu Kothari (:ritu) from comment #13)
> Hi Gijs, I know the team has triaged this as a P3. Does that mean it will
> get fixed in 57? I hope it does. I tried the STR and this bug is kinda
> sloppy :(

Jeff, need product to way in here, should this block 57?
Flags: needinfo?(jmathies) → needinfo?(jgriffiths)
(In reply to Jim Mathies [:jimm] from comment #30)
> (In reply to Ritu Kothari (:ritu) from comment #13)
> > Hi Gijs, I know the team has triaged this as a P3. Does that mean it will
> > get fixed in 57? I hope it does. I tried the STR and this bug is kinda
> > sloppy :(
> 
> Jeff, need product to way in here, should this block 57?

(In reply to Ritu Kothari (:ritu) from comment #28)
> I'd like to add that this bug also seems fixed for me (tested on 57.0b6),
> similar to Gij's experience in comment c16.

Jim, based on this last comment from Ritu I'd say not, if there is no longer a repro.
Flags: needinfo?(jgriffiths)
I can still reproduce in latest Nightly 58.0a1 (2017-10-18) and Beta 57.0b9.
Attached image flicker.gif — 
Here's what I see from OP's STR today. A brief flicker before showing expected results, rather than getting stuck.
Eh, you can't see it in that GIF because of the low framerate. You can kinda see it in this screencast: http://recordit.co/2RF774DYyV
I want to point out that the reported bug is on Windows 7, just as the other related duplicated bugs.

Quoting mconley in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1402028#c0 :
> I can reproduce this reliably on my Windows 7 machine.
> On my Windows 10 machine, the Help subview displays for a short time, and then is replaced by the default view for the app menu.

I use nightly every day on a Windows 7 computer that I update manually using menu->help->about.
No patch of other bugs have yet fixed or mitigated this bug since it was introduced, at least on this platform.

With today's nightly 58.0a1 20171024100135:
  - I can confirm the flickering on Windows 10
  - sub-menus are still stuck on Windows 7

This bug is also reproducible with 57 Beta 11 and Developer Edition.
I can reproduce this too now on a stock downloaded Nightly on Windows 7 SP1 (all important updates installed).

This really needs someone from gfx to take a look, because it looks like we're invalidating the region way too lazily.
Milan, can you find someone to take a look at this win7 issue?
Flags: needinfo?(milan)
I can confirm that the issue mentioned in comment 0 is still reproducible only on Windows 7 64bit machines with 57.b11.

The previously opened sub menus are still displayed when the "Hambuger" menu is reopened.

Jeff, is this behavior changing this particular issue into a "blocker" for 57 (since this issue is still reproducible)?

Thanks!
Flags: needinfo?(jeff)
(In reply to Mike de Boer [:mikedeboer] from comment #37)
> I can reproduce this too now on a stock downloaded Nightly on Windows 7 SP1
> (all important updates installed).
> 
> This really needs someone from gfx to take a look, because it looks like
> we're invalidating the region way too lazily.

I agree, this definitely needs to be evaluated to understand how to fix it.

(In reply to Emil Ghitta, QA [:emilghitta] from comment #39)
...
> Jeff, is this behavior changing this particular issue into a "blocker" for
> 57 (since this issue is still reproducible)?

This bug does not cause a crash, introduce a performance problem or impact an area of the UI that most users access so from my point of view this should not block. This is an unfortunate polish bug.

 If there was a very low-risk patch that fixed this issue and the date was 2 weeks ago I would be more interested in trying to get it in. Currently we have no root cause identified and only 2 weeks to go before release, so I would not be supportive of an uplift of a fix.

*Currently we do not even have a fix.*
Flags: needinfo?(jeff)
I've noticed that this behaviour on Windows 10 x64 when I was using beta 57.12. I checked on latest Nightly (2017-10-30) and the bug was reproducing there too. 
I ran mozregression and found out that the bug 1374749 might cause the problem.

last good build: 2017-09-12
first bad build: 2017-09-13

I checked on Windows 8.1 x64, just to be sure, and the bug was reproducing there as well on the latest Nightly.
OS: Windows 7 → Windows
FYI, specific and detailed regression range was already posted long time ago in Comment #0. Furthermore, it's still reproducible in latest Mozilla Firefox Nightly 58.0a1 (2017-10-30), as still no one is assigning to it and no other related bug fix this one, so there is no need for more confirmation, that it still reproducible. Thanks. ;)
Hard to see how we can move this in 57, especially if it is a low level change to do with invalidation or something like that.
Flags: needinfo?(milan)
I'm going to take a look. If I can't find a fix quickly or the fix isn't really simple, we shouldn't uplift it.
Flags: needinfo?(mstange)
This is a bug in the compositor. A container layer is holding on to its intermediate surface even though its child layers have changed.
(In reply to Markus Stange [:mstange] from comment #46)
> This is a bug in the compositor. A container layer is holding on to its
> intermediate surface even though its child layers have changed.

This sounds like if we ensured that the container layer was also updated somehow (via CSS) we could workaround in the frontend. Is there an easy way of doing this, esp. if fixing the compositor is hard?
Flags: needinfo?(mstange)
I'm currently trying to figure that out. I haven't understood the bug completely yet.
Flags: needinfo?(mstange)
Flags: needinfo?(matt.woodrow)
I found a workaround. I think it's low-risk enough to uplift to 57, if that is still an option. I'm going to kick off a try build so that others can verify that the workaround works.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Priority: P3 → P1
Depends on: 1414033
Comment on attachment 8924688 [details]
Bug 1400259 - Add will-change:transform to the panel in order to work around a compositor bug.

https://reviewboard.mozilla.org/r/195914/#review201132

Thanks!

::: browser/base/content/browser.css:1175
(Diff revision 1)
>  
>  %elifndef MOZ_WIDGET_GTK
>  
>  #BMB_bookmarksPopup:not([animate="false"]) {
>    opacity: 0;
> +  will-change: transform;

Can you add a comment here, and in the other place where you added `will-change`, that we need this for Windows to work around the compositor bug? (And perhaps file a new bug for fixing the compositor bug, and reference it from the comment)
Attachment #8924688 - Flags: review?(mconley) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/66ef885f0b66
Add will-change:transform to the panel in order to work around a compositor bug. r=mconley
The win64 opt build on the autoland push is now available: https://queue.taskcluster.net/v1/task/cH2wAzVkQYi4Eqm77Whabw/runs/0/artifacts/public/build/target.zip
Can you please test this build and check whether all instances of this bug are gone? Thank you!
Flags: needinfo?(Virtual)
Testing Markus' build on Windows 7:

Menu > Help, cancel menu.  Reopen menu: Main menu is displayed. [fixed]

Menu > Help > About Nightly, close About dialog.  Reopen menu: Help menu is displayed. [not fixed]

Library > Screenshots.  Reopen menu: Library menu is displayed. [not fixed]

Library > History > Recently Closed Tabs, cancel menu.  Reopen menu: Main menu is displayed. [fixed]

Library > History > Recently Closed Tabs, reopen a tab.  Reopen menu: Recently Closed Tabs menu is displayed. [not fixed]


Current Nightly results:

Menu > Help, cancel menu.  Reopen menu: Help menu is displayed.

Menu > Help > About Nightly, close About dialog.  Reopen menu: Help menu is displayed.

Library > Screenshots.  Reopen menu: Library menu is displayed.

Library > History > Recently Closed Tabs, cancel menu.  Reopen menu: Recently Closed Tabs menu is displayed.

Library > History > Recently Closed Tabs, reopen a tab.  Reopen menu: Recently Closed Tabs menu is displayed.


Summary:

It fixes instances of returning to the submenu where you cancel the menu and reopen the menu, but does not fix instances where you select an item from a submenu.
Note: the above "Library > History"/etc should be "Menu > Library > History"/etc.
Thank you!
Keywords: leave-open
Attachment #8924688 - Attachment is obsolete: true
(In reply to Markus Stange [:mstange] from comment #54)
> The win64 opt build on the autoland push is now available:
> https://queue.taskcluster.net/v1/task/cH2wAzVkQYi4Eqm77Whabw/runs/0/
> artifacts/public/build/target.zip
> Can you please test this build and check whether all instances of this bug
> are gone? Thank you!

I'm confirming that this bug it's fixed partially with that build
and builds starting from Mozilla Firefox Nightly 58.0a1 (2017-11-03).
Detailed explanation about this by David Smith is in comment #55 and comment #56.
Looking that patch for leftovers is attached too,
I have nothing more to add, so I'm clearing :ni :)
Flags: needinfo?(Virtual)
Attachment #8925141 - Flags: review?(mconley) → review?(gijskruitbosch+bugs)
Comment on attachment 8925141 [details]
Bug 1400259 - Extend the workaround to cover all popup states and both opacity and transform.

https://reviewboard.mozilla.org/r/196392/#review201870

Stealing. I tested this on my win7 VM and can verify that the remaining unfixed issues are fixed for me. The patch seems sane, small and contained to me, so let's do it. If this could still make the RC that would be Really Nice.
Attachment #8925141 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/80a2fea82e8f
Extend the workaround to cover all popup states and both opacity and transform. r=Gijs
Is there more work needed on this bug, or should the leave-open keyword be cleared now (and the bug closed)?  Comment 61 seems to imply we're good now.
Flags: needinfo?(mstange)
(In reply to Julien Cristau [:jcristau] from comment #64)
> Is there more work needed on this bug, or should the leave-open keyword be
> cleared now (and the bug closed)?  Comment 61 seems to imply we're good now.

It would probably still be good to be 100% sure that this is now also gone for David and Virtual_ManPL. (Ni for this; x64 Windows builds for testing: https://queue.taskcluster.net/v1/task/I1PT5VBESkaIJVFnCZMVIQ/runs/0/artifacts/public/build/target.zip ; Win32: https://queue.taskcluster.net/v1/task/Rs6C8RmIQqigJgZhzTrtsg/runs/0/artifacts/public/build/target.zip )

Whether this can be closed is up to Markus - not sure if there are remaining issues in the compositor/layout/graphics that he wants to flush out here. But I do believe the core reported issue is no longer reproducible (I'd just be happy to have it confirmed by someone else still, too... bit paranoid about these panel polish issues at this stage).
Flags: needinfo?(dsmith)
Flags: needinfo?(Virtual)
(In reply to :Gijs (slow, PTO recovery mode) from comment #65)
> x64 Windows builds for testing:
> https://queue.taskcluster.net/v1/task/I1PT5VBESkaIJVFnCZMVIQ/runs/0/artifacts/public/build/target.zip

In my case, I can't reproduce anymore the issue with all STRs with this build.
So bug looks completely fixed.
Thank you very much! \o/
Flags: needinfo?(Virtual)
Thanks for testing!

Yes, this can be closed, I forgot to remove leave-open.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Flags: needinfo?(mstange)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Approval Request Comment
[Feature/Bug causing the regression]: bug 1374749
[User impact if declined]: very annoying visual glitch when opening the Hamburger menu
[Is this code covered by automated tests?]: We have tests for panels but they don't inspect the pixels that are rendered by the panels on the screen. This particular bug happens very late in the painting pipeline which is hard to capture.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: QE is welcome to test some more, but this fix has been tested extensively by at least three people (myself, Gijs, VirtualMan)
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a very targeted, CSS-only fix.
[String changes made/needed]: none
Attachment #8925992 - Flags: approval-mozilla-beta?
If we're trying to land this on 57 still, does this want release approval as well? I think rc 1 got spun yesterday...
status-firefox57: wontfix → ?
Flags: needinfo?(lhenry)
I'm also confirming that bug is fixed, starting in Mozilla Firefox Nightly 58.0a1 (2017-11-07), so I'm marking this bug as VERIFIED.
Thank you very much one more time! \o/
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Comment on attachment 8925992 [details] [diff] [review]
combined patch for 57

see comment 68
Attachment #8925992 - Flags: approval-mozilla-release?
Ritu is managing the 57 release.
Flags: needinfo?(lhenry) → needinfo?(rkothari)
This is awesome! Thank you for fixing this one. \o/ I have asked for a second opinion from Milan, Justin and Milan has confirmed this would be a good one to uplift to 57. Unless I hear otherwise, I will take it in 57 RC2.
status-firefox57: ? → affected
Flags: needinfo?(rkothari)
Comment on attachment 8925992 [details] [diff] [review]
combined patch for 57

This is a low risk, high reward fix, Beta57+
Attachment #8925992 - Flags: approval-mozilla-release?
Attachment #8925992 - Flags: approval-mozilla-release+
Attachment #8925992 - Flags: approval-mozilla-beta?
Attachment #8925992 - Flags: approval-mozilla-beta+
Verified in Firefox 57.0 build 3 under Win 7 and Win 10 64-bit as part of build sign-off.
Flags: qe-verify+
Flags: needinfo?(dsmith)
Depends on: 1422611
Depends on: 1426317
Depends on: 1462857
Flags: in-qa-testsuite+
Regressions: 1543508
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: