Closed Bug 1206469 Opened 4 years ago Closed 4 years ago

Bookmarks submenu of bookmarks widget is closed immediately during dragging (on Windows7 Classic only)

Categories

(Firefox :: Theme, defect)

38 Branch
Unspecified
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
firefox40 --- wontfix
firefox41 - wontfix
firefox42 + verified
firefox43 + verified
firefox44 --- verified
firefox-esr38 --- affected

People

(Reporter: alice0775, Assigned: Paenglab)

References

Details

(Keywords: regression, ux-consistency)

Attachments

(1 file)

This problem occurs on Windows7 Classic only.
This problem occurs in Bookmarks Widget only. 
This problem does not occur in Bookmarks Menubar and Toolbar.

Regressed on Firefox 38.

Steps to reproduce:
0. Make sure Windows7 Classic
1. Drag a link into submenu of Bookmarks Widget slowly

Actual results:
The submenu is closed.

Expected results:
The submenu should be staying.


Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=134bcefd5f7b&tochange=6fbc7d37e16b

Regressed by: 6fbc7d37e16b	Richard Marti — Bug 1079098 - Add a visible hover feedback on panelUI with high contrast theme. r=Gijs
Flags: needinfo?(richard.marti)
Hmm, I don't see how only color changes could make such an effect.
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #1)
> Hmm, I don't see how only color changes could make such an effect.

We added borders and that will change sizes. Can you look into this a bit more? Can you reproduce, for example, and maybe does changing the border width to 0 work around the issue?
Flags: needinfo?(richard.marti)
Summary: Bookmarks submenu of bookmarks wifget is closed immediately during dragging (on Windows7 Classic only) → Bookmarks submenu of bookmarks widget is closed immediately during dragging (on Windows7 Classic only)
Checked the issue and can confirm it.

The strange thing is, it isn't a border definition which is causing this. It's the removing of a box-shadow.

https://dxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/customizableui/panelUIOverlay.css#30 adds a box-shadow and I removed it with https://dxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/customizableui/panelUIOverlay.css#225.

Also strange something like box-shadow: 0 0 4px transparent; or box-shadow: 0 0 4px rgba(0,0,0,0.01); shows the issue but box-shadow: 0 0 4px rgba(0,0,0,0.02); not. For me this looks like a bug in the box-shadow implementation.

Gijs, what do you think, what should be the best? Should I remove the box-shadow line or should this be fixed in the box-shadow implementation? Leaving the shadow is not so bad for the high contrast themes.
Flags: needinfo?(richard.marti) → needinfo?(gijskruitbosch+bugs)
(In reply to Richard Marti (:Paenglab) from comment #3)
> Checked the issue and can confirm it.
> 
> The strange thing is, it isn't a border definition which is causing this.
> It's the removing of a box-shadow.
> 
> https://dxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/
> customizableui/panelUIOverlay.css#30 adds a box-shadow and I removed it with
> https://dxr.mozilla.org/comm-central/source/mozilla/browser/themes/windows/
> customizableui/panelUIOverlay.css#225.
> 
> Also strange something like box-shadow: 0 0 4px transparent; or box-shadow:
> 0 0 4px rgba(0,0,0,0.01); shows the issue but box-shadow: 0 0 4px
> rgba(0,0,0,0.02); not. For me this looks like a bug in the box-shadow
> implementation.

Wat. This is quite strange. dholbert, any chance you can look into what on earth is going on here?

As to using a workaround like keeping the unwanted box-shadow, I would rather fix whatever is breaking this in layout/xul land than fudge the CSS to avoid the problem.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dholbert)
[Tracking Requested - why for this release]:   Ritu, just letting you know it's a regression noticeable by Win7 users at least, so you may want to track and follow up.
box-shadow changes do currently cause reflow (and I don't know exactly how menus work, but I'm willing to believe that this would explain the observed behavior). But coincidentally, I'm about to fix that [making box-shadow only cause a repaint, basically] in bug 1194480.
Flags: needinfo?(dholbert)
OK, let's mark a dep and come back here after that work lands.

In the meantime, I've changed my mind regarding the workaround, having tested this on win7 myself. Dragging "slowly" is descriptive, but it's not just "super slow" drags that affect this. Additionally I'm not sure how upliftable the box-shadow stuff will be (it seems to be something you've been working on a while already, dholbert?) or if that will improve or exacerbate this problem.

Richard, any chance you could work up that patch?
Depends on: 1194480
Flags: needinfo?(richard.marti)
Attached patch Bug1206469.patchSplinter Review
Is it okay to leave the old rule with the comment in the file?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Flags: needinfo?(richard.marti)
Attachment #8664364 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8664364 [details] [diff] [review]
Bug1206469.patch

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

::: browser/themes/windows/customizableui/panelUIOverlay.css
@@ +224,5 @@
>  
>    #BMB_bookmarksPopup menupopup[placespopup=true] > hbox {
> +    /* After fixing of bug 1194480 the box-shadow can be removed again */
> +    /* box-shadow: none; */
> +    box-shadow: 0 0 4px rgba(0,0,0,0.02);

But this is the same as it already is, right? Can we just not re-set it again here? Is there some reason that doesn't work?
Attachment #8664364 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8664364 [details] [diff] [review]
> Bug1206469.patch
> 
> Review of attachment 8664364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/customizableui/panelUIOverlay.css
> @@ +224,5 @@
> >  
> >    #BMB_bookmarksPopup menupopup[placespopup=true] > hbox {
> > +    /* After fixing of bug 1194480 the box-shadow can be removed again */
> > +    /* box-shadow: none; */
> > +    box-shadow: 0 0 4px rgba(0,0,0,0.02);
> 
> But this is the same as it already is, right? Can we just not re-set it
> again here? Is there some reason that doesn't work?
Flags: needinfo?(richard.marti)
(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to :Gijs Kruitbosch from comment #9)
> > Comment on attachment 8664364 [details] [diff] [review]
> > Bug1206469.patch
> > 
> > Review of attachment 8664364 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/themes/windows/customizableui/panelUIOverlay.css
> > @@ +224,5 @@
> > >  
> > >    #BMB_bookmarksPopup menupopup[placespopup=true] > hbox {
> > > +    /* After fixing of bug 1194480 the box-shadow can be removed again */
> > > +    /* box-shadow: none; */
> > > +    box-shadow: 0 0 4px rgba(0,0,0,0.02);
> > 
> > But this is the same as it already is, right? Can we just not re-set it
> > again here? Is there some reason that doesn't work?

No, on line 30 it's 0.2 and I set 0.*0*2 to make it more transparent. 0.01 is too much and the bug remains.
Flags: needinfo?(richard.marti)
Comment on attachment 8664364 [details] [diff] [review]
Bug1206469.patch

D'oh. Sorry! Then yes, let's do this.
Attachment #8664364 - Flags: review+
Keywords: checkin-needed
Flags: qe-verify+
It is unfortunate that we have this bug but given the workaround (or other ways) of doing the same task I am a bit less concerned. Also in the description it is mentioned that this is an issue since FF38. I think we will have to live with this regression through the 41 cycle rather than take a fix in a dot release. It is too late and this is now wontfix for 41.

Let's try to fix this in 42 cycle.
https://hg.mozilla.org/mozilla-central/rev/0e1fd61689c6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Richard, could you fill the uplift request to aurora & beta? Thanks
Flags: needinfo?(richard.marti)
Comment on attachment 8664364 [details] [diff] [review]
Bug1206469.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Submenus in bookmark widget not usable with Classic/HC themes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c
[Risks and why]: Low, only CSS change.
[String/UUID change made/needed]: No
Flags: needinfo?(richard.marti)
Attachment #8664364 - Flags: approval-mozilla-beta?
Attachment #8664364 - Flags: approval-mozilla-aurora?
Comment on attachment 8664364 [details] [diff] [review]
Bug1206469.patch

Thanks, taking it into 42, should be in beta 3.
Attachment #8664364 - Flags: approval-mozilla-beta?
Attachment #8664364 - Flags: approval-mozilla-beta+
Attachment #8664364 - Flags: approval-mozilla-aurora?
Attachment #8664364 - Flags: approval-mozilla-aurora+
I was able to reproduce this issue on Firefox 43.0a1 (2015-09-20) under Windows 7 64-bit Classic.
Verified fixed on Firefox 44.0a1 (2015-09-30), Firefox 43.0a2 (2015-09-30) under Windows 7 64-bit Classic.
Status: RESOLVED → VERIFIED
Also verified fixed on Firefox 42 Beta 3 (20151001142456) under Windows 7 64-bit Classic.
We can back out this change on Nightly now that bug 1194480 has landed, right?
Flags: needinfo?(richard.marti)
Flags: needinfo?(dholbert)
Yes, checked a backout and it still works.

Should I file a new bug for backout add the patch here?
Flags: needinfo?(richard.marti) → needinfo?(gijskruitbosch+bugs)
This should be:

Should I file a new bug for backout *or* add the patch here?
Given that this was backported, it'd be saner from a tracking perspective to do the backout in its own bug, IMO. (Particularly if the backout ends up causing a regression or other side-effects, and we need to represent that with blocking/depends-on fields)
Depends on: 1211250
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.