985958 - Bookmarks toolbar open bookmark folder background depends on hover state when it shouldn't

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[RobertJ]

Attachment: Screen Shots showing issue

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140318013849

Steps to reproduce:

Go to folder on bookmark toolbar

1 - Hover
2 - Click & Hold
3 - Click with cursor in dropdown
4 - Click with cursor out of dropdown


Actual results:

1 - Hover - Light gray background
2 - Click & Hold - Dark gray background
3 - Click with cursor in dropdown  - Light gray background
4 - Click with cursor out of dropdown - Dark gray background

See attached file.

This occurs on FF29b, FF30a2 and FF31a1.  I "think" I first noticed this when FF29 was in the Aurora channel but not sure.


Expected results:

1 - Hover - Light gray background
2 - Click & Hold - Dark gray background
3 - Click with cursor in dropdown  - DARK GRAY BACKGROUND
4 - Click with cursor out of dropdown - Dark gray background

Comment 1

[:Gijs (he/him)]

This doesn't seem like a terribly big deal to me, but yes, we should ideally fix it.

Comment 2

[RobertJ]

Here is the regression range

3-13-14 NO BUG
3-14-14 BUG THERE

Comment 3

[RobertJ]

(In reply to :Gijs Kruitbosch from comment #1)
> This doesn't seem like a terribly big deal to me, but yes, we should ideally
> fix it.

I agree "it doesn't seem" like a big deal; however, on a Mac it looks like the bookmark folder background FLASHES. It is rather annoying.

Hopefully with the regression range and knowledge of the code architecture (which I don't have), it will require only a few minutes to identify the "guilty" CSS.

Cheers all

Comment 4

[RobertJ]

Here is a hack to fix the bug. I inserted this in userChrome.css

#PlacesToolbarItems > .bookmark-item[open="true"] {
background-color: #555555 !important;
}

Now if this is inserted/modified in the actual code; all should be good.

At least for now the bookmark folder background is not FLASHING.

Cheers again and have a good weekend.

Comment 5

[RobertJ]

Anyone care to furnish the pushlog for this regression?

Comment 6

[:Gijs (he/him)]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46041cc216fd&tochange=f073b3d6db1f

Comment 7

[:Gijs (he/him)]

I broke this in bug 982835, apparently. I don't really understand why, though. :-\

Comment 8

[:Gijs (he/him)]

Oh, yes I do - the [open] selector gets overridden by the :hover one because of the extra :not(.subviewbutton). Sigh.

Comment 9

[RobertJ]

So a fix is on the way? 

I had found this range of code using the DOMi but had no idea what other things were involved.

Simple fix, yes?

Comment 10

[:Gijs (he/him)]

Attachment: fix CSS rules to make :active style work,

So I don't understand why we have two identical selectors that apply 'hover' and 'active' styling to [open] things, but I just got rid of the one that gets overridden, and adjusted the one that shouldn't be overridden by the actual ':hover' selector so that it doesn't. This fixes things for me. Mike, am I sane (at least as far as this patch goes)?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8398732 [details] [diff] [review]
fix CSS rules to make :active style work,

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

Nutty. I'm tempted to go back through the rev log and see what happened here, but I'm gonna guess an unintended collision of old and new code.

This looks better. Let's go for it.

Comment 12

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/822b3855dc4b

Comment 13

[:Gijs (he/him)]

Comment on attachment 8398732 [details] [diff] [review]
fix CSS rules to make :active style work,

(I am not around for a week after today, so pre-emptively asking for approval)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / bug 982835
User impact if declined: bookmark folders have strange 'open' styling
Testing completed (on m-c, etc.): on m-c, locally
Risk to taking this patch (and alternatives if risky): low. Can't really get worse than it is right now. :-\
String or IDL/UUID changes made by this patch: none

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/822b3855dc4b

Comment 15

[RobertJ]

Sorry but not familiar with how this works. Nightly is definitely fixed; when will the fix land on 30Aurora and 29Beta?

Comment 16

[Mike de Boer [:mikedeboer]]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/0d716cd5c4dd
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/2a9de2deeb0a

Comment 17

[RobertJ]

Thanks. I'll watch for them to show up.

Comment 18

[Ada [:adalucinet]]

Reproduced with Nightly from 2014-03-20. 
Verified as fixed with latest Nightly (Build ID: 20140401030203), latest Aurora (Build ID: 20140402004007) and Firefox 29 beta 4 (Build ID: 20140331125246) on Mac OS X 10.9.2:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0