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

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[Alice0775 White]

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

Comment 1

[Richard Marti (:Paenglab)]

Hmm, I don't see how only color changes could make such an effect.

Comment 2

[:Gijs (he/him)]

(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?

Comment 3

[Richard Marti (:Paenglab)]

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.

Comment 4

[:Gijs (he/him)]

(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.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

[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.

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[:Gijs (he/him)]

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?

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Bug1206469.patch

Is it okay to leave the old rule with the comment in the file?

Comment 9

[:Gijs (he/him)]

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?

Comment 10

[:Gijs (he/him)]

(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?

Comment 11

[Richard Marti (:Paenglab)]

(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.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8664364 [details] [diff] [review]
Bug1206469.patch

D'oh. Sorry! Then yes, let's do this.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0e1fd61689c6

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e1fd61689c6

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Richard, could you fill the uplift request to aurora & beta? Thanks

Comment 17

[Richard Marti (:Paenglab)]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8664364 [details] [diff] [review]
Bug1206469.patch

Thanks, taking it into 42, should be in beta 3.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9ac72490a8cc

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1167c42d21e4

Comment 21

[Vasilica Tamas, QA [:vtamas]]

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.

Comment 22

[Vasilica Tamas, QA [:vtamas]]

Also verified fixed on Firefox 42 Beta 3 (20151001142456) under Windows 7 64-bit Classic.

Comment 23

[:Gijs (he/him)]

We can back out this change on Nightly now that bug 1194480 has landed, right?

Comment 24

[Daniel Holbert [:dholbert]]

I'd expect so, yes.

Comment 25

[Richard Marti (:Paenglab)]

Yes, checked a backout and it still works.

Should I file a new bug for backout add the patch here?

Comment 26

[Richard Marti (:Paenglab)]

This should be:

Should I file a new bug for backout *or* add the patch here?

Comment 27

[Daniel Holbert [:dholbert]]

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)