Reproducible on: Nightly 38.0a1 (2015-02-17). Affected platform(s): Windows 8.1 64 bit, Ubuntu 14.04 64 bit, Mac OS X 10.9.5. Steps to reproduce: 1. Launch Firefox with a clean profile. 2. Make sure that 'reader.parse-on-load.enabled' is set to true in about:config. 3. Access a random article from the web - e.g. http://www.bbc.com/future/story/20150211-why-does-popcorn-pop. 4. Press [CTRL] / [CMD] + [L] to bring the Location Bar into focus. Don't use your mouse or touchpad. 5. Press [TAB] to select the Reader Mode button and then press either [SPACE] or [RETURN]. 6. Repeat the two previous steps to disable Reader Mode and press [CTRL] / [CMD] + [L] before the page reloads. Expected result: 5. The Reader Mode button turns orange, showing that it's enabled. 6. The Reader Mode button turns gray, showing that it's disabled. Actual result: 6. The Reader Mode button remains orange, although it's no longer enabled.

Timothy, this seems like a layout/graphics issue to me. Do you know who would be the best person to look at this?

I tried several times and I was not able to reproduce this on my mac laptop. I tested in nightly and I tried in my normal testing profile, and also in a fresh profile, both with and without e10s. Without being able to reproduce I can't really say where the problem might be, but I can ask some questions to try to narrow it down. When one can reproduce, what happens when the focus leaves the location bar (does it still draw incorrectly)? What if you minimize and restore the window? Or focus another app and then come back to firefox. How is the reader icon implemented? <img> element? Background image? How is orange/grey toggled? class selector? The bug title indicates this is the hover state, but when I hover the icon it doesn't change color, so I'm a little confused about that. (not sure who exactly to needinfo here)

(In reply to Timothy Nikkel (:tn) from comment #2) > I tried several times and I was not able to reproduce this on my mac laptop. > I tested in nightly and I tried in my normal testing profile, and also in a > fresh profile, both with and without e10s. Yeah, margaret also couldn't reproduce, whereas I could and Jared could, too. I wonder what the differences are between our machines, and if it has to do with HWA or something or other... :-\ > Without being able to reproduce I > can't really say where the problem might be, but I can ask some questions to > try to narrow it down. > > When one can reproduce, what happens when the focus leaves the location bar Focus is put on the content document anyway, and this doesn't seem to affect the color of the item. I can go and move focus through it (from the search box to the button to the location bar) and this doesn't seem to affect anything - as long as I don't move my mouse. > (does it still draw incorrectly)? Yes, see above. > What if you minimize and restore the > window? Still incorrect. > Or focus another app and then come back to firefox. Still incorrect. However, hitting just <alt> to make the menubar appear (on Windows) makes the icon redraw. > How is the > reader icon implemented? <img> element? Background image? How is orange/grey > toggled? class selector? The bug title indicates this is the hover state, > but when I hover the icon it doesn't change color, so I'm a little confused > about that. It's a <toolbarbutton> with a list-style-image style that points to an image. All the relevant CSS is here: http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#1579 1579 /* Reader mode button */ 1580 1581 #reader-mode-button { 1582 -moz-appearance: none; 1583 padding: 0; 1584 list-style-image: url("chrome://browser/skin/reader-mode-16.png"); 1585 -moz-image-region: rect(0, 16px, 16px, 0); 1586 } 1587 1588 #reader-mode-button > .toolbarbutton-icon { 1589 width: 16px; 1590 } 1591 1592 #reader-mode-button:focus { 1593 outline: 1px dotted; 1594 } 1595 1596 #reader-mode-button:hover:active, 1597 #reader-mode-button[readeractive] { 1598 -moz-image-region: rect(0, 32px, 16px, 16px); 1599 }

This appears to be a style system bug. The new style context for the frame does not have the right values. Investigating.

This could be "fixed" (i.e. worked around, if the underlying issue is a style system bug) by fixing bug 1140340 and reverting most of bug 1131458 (except for the tooltip).

Actually I made a mistake in the testcase I created: the testcase does not show any bug at all, it is behaving as expected. My previous statement no longer stands. And I am back to not being able to reproduce. And I tried on Windows too. Not really sure how to proceed here :( I assume you've verified that the attributes on the button are set correctly when the bug appears? Does the bug happen if you removed the hover and/or active selectors?

(In reply to Timothy Nikkel (:tn) from comment #6) > Actually I made a mistake in the testcase I created: the testcase does not > show any bug at all, it is behaving as expected. My previous statement no > longer stands. And I am back to not being able to reproduce. And I tried on > Windows too. Not really sure how to proceed here :( > > I assume you've verified that the attributes on the button are set correctly > when the bug appears? Does the bug happen if you removed the hover and/or > active selectors? I've not checked, but I can tell you that when this happens, this: setTimeout(() => console.log(document.getElementById("reader-mode-button").matches(":hover")), 10000) prints true, so it definitely seems like it's related to the hover state being there when it shouldn't (esp. because all my interactions with the button were via the keyboard). I also noticed that on OS X, hitting cmd-` to switch to the browser console makes the issue go away...

On OS X, with e10s enabled, on a clean profile with reader mode enabled, I can still repro this with the following steps: 1. create profile, start, dismiss 'set as default browser' prompt 2. use about:config to set reader.parse-on-load.enabled to true 3. go to news.bbc.co.uk and open a random article that triggers the reader mode button 4. cmd-l, tab, space 5. after the page reloads in reader mode: cmd-l, tab, space 6. wait for the page to reload. I can also reproduce in safe mode (which dismisses the HWA theory, I guess).

I can finally reproduce with those steps, seems like having the mouse over the Firefox window is important.

Looks like the code in nsButtonBoxFrame::HandleEvent is responsible. It sets active and hover on key down for the space bar, and then removes it on key up for the spacebar. But it looks like the key up never gets delivered to the frame because the focus gets changed to the iframe containing the content document. Not really sure how that should work.

(In reply to Timothy Nikkel (:tn) from comment #10) > Looks like the code in nsButtonBoxFrame::HandleEvent is responsible. It sets > active and hover on key down for the space bar, and then removes it on key > up for the spacebar. But it looks like the key up never gets delivered to > the frame because the focus gets changed to the iframe containing the > content document. Not really sure how that should work. Seems like the focus would be changing because the node gets hidden, right? Or at least, the node does (temporarily) get hidden completely by the reader code, so at a minimum, when that happens, it seems like the hover/active state should be cleared then? I can't think of any case where you can keep hovering/"activating" something that gets display:none'd.

(In reply to :Gijs Kruitbosch from comment #11) > Seems like the focus would be changing because the node gets hidden, right? > Or at least, the node does (temporarily) get hidden completely by the reader > code, so at a minimum, when that happens, it seems like the hover/active > state should be cleared then? I can't think of any case where you can keep > hovering/"activating" something that gets display:none'd. There isn't an obvious place to capture "being hidden" and perform actions based on it. The problem is that frames providing their own event handling implementation for things like active/hover is just problematic, it should be done by the event state manager code instead of custom code for this one frame type.

Bug 1132678 has been backed out. (In reply to :Gijs Kruitbosch from comment #11) > Seems like the focus would be changing because the node gets hidden, right? bug 570835

Assuming you are going a different direction and no longer need this.

(In reply to Timothy Nikkel (:tn) from comment #14) > Assuming you are going a different direction and no longer need this. This still applies to the reader mode button, and it's not clear we will change that (cf. bug 1140340 comment 10). (In reply to Timothy Nikkel (:tn) from comment #12) > (In reply to :Gijs Kruitbosch from comment #11) > > Seems like the focus would be changing because the node gets hidden, right? > > Or at least, the node does (temporarily) get hidden completely by the reader > > code, so at a minimum, when that happens, it seems like the hover/active > > state should be cleared then? I can't think of any case where you can keep > > hovering/"activating" something that gets display:none'd. > > There isn't an obvious place to capture "being hidden" and perform actions > based on it. The problem is that frames providing their own event handling > implementation for things like active/hover is just problematic, it should > be done by the event state manager code instead of custom code for this one > frame type. Is this difficult to address?

smaug might be able to better answer how this should be implemented.

Hmm, nsButtonBoxFrame has such odd code. Oh well, it could just have a blur listener and clear the state when blur is handled, I think.

Good idea. How does one do that? nsButtonBoxFrame::HandleEvent doesn't seem to get NS_BLUR_CONTENT event ever.

that ::HandleEvent is for the cases when a callback is passed to EventDispatcher::Dispatch, so in general for those events which come from widget level to presshell before being dispatched to DOM. focus management is a separate thing and doesn't rely on layout stuff, so you'd need to just have an object implementing nsIDOMEventListener and add the listener to system event group. Similar to ... say...nsMenuBarFrame and its mMenuBarListener

Hmm, actually the sequence of events goes like this key down event, for space key blur the button key press event (no keycode, I guess that's normal?) and then sometimes the button box frame get the key up for the space bar, sometimes we don't. Not sure why that happens sometimes but not others.

Comment on attachment 8583549 [details] [diff] [review] patch > void >+nsButtonBoxFrame::Init(nsIContent* aContent, >+ nsContainerFrame* aParent, >+ nsIFrame* aPrevInFlow) >+{ >+ nsBoxFrame::Init(aContent, aParent, aPrevInFlow); >+ >+ mButtonBoxListener = new nsButtonBoxListener(this); >+ NS_ADDREF(mButtonBoxListener); Use nsRefPtr<nsButtonBoxListener> in the class and no manual ADDREF/RELEASE >+mContent->AddEventListener(NS_LITERAL_STRING("blur"), mButtonBoxListener, false); You should add the listener to system event group, so, AddSystemEventListener >+nsButtonBoxFrame::DestroyFrom(nsIFrame* aDestructRoot) >+{ >+ mContent->RemoveEventListener(NS_LITERAL_STRING("blur"), mButtonBoxListener, false); Please explicitly clear nsButtonBoxListener::mButtonBoxFrame here and add a null check to nsButtonBoxListener::HandleEvent

Comment on attachment 8584221 [details] [diff] [review] patch v2 Now I started to wonder... since all this state handling is very unique and odd... should we clear the state only if keyboard actually set the state. I think so. So add a boolean variable to nsButtonBoxListener and set it true in case NS_KEY_DOWN and false in case NS_KEY_UP. Then in nsButtonBoxListener::HandleEvent handle the event only if the flag is true. (and once handling the event, set it to false again.) With that, r+.

Added the bool and pushed. https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd0338c09a4

Lost header include in rebase https://hg.mozilla.org/integration/mozilla-inbound/rev/861900f9685a

https://hg.mozilla.org/mozilla-central/rev/4bd0338c09a4 https://hg.mozilla.org/mozilla-central/rev/861900f9685a

Verified fixed on Aurora 39.0a2 (2015-04-02), using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.