1133732 - Reader Mode button remains in active/hover state after toggling it

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Andrei Vaida [:avaida]]

Attachment: screenshot from Ubuntu 14.04 x64

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.

Comment 1

[:Gijs (he/him)]

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

Comment 2

[Timothy Nikkel (:tnikkel)]

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)

Comment 3

[:Gijs (he/him)]

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

Comment 4

[Timothy Nikkel (:tnikkel)]

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

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[Timothy Nikkel (:tnikkel)]

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?

Comment 7

[:Gijs (he/him)]

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

Comment 8

[:Gijs (he/him)]

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

Comment 9

[Timothy Nikkel (:tnikkel)]

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

Comment 10

[Timothy Nikkel (:tnikkel)]

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.

Comment 11

[:Gijs (he/him)]

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

Comment 12

[Timothy Nikkel (:tnikkel)]

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

Comment 13

[Dão Gottwald [:dao]]

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

Comment 14

[Timothy Nikkel (:tnikkel)]

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

Comment 15

[:Gijs (he/him)]

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

Comment 16

[Timothy Nikkel (:tnikkel)]

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

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Timothy Nikkel (:tnikkel)]

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

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 20

[Timothy Nikkel (:tnikkel)]

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 21

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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 23

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 25

[Timothy Nikkel (:tnikkel)]

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

Comment 26

[Timothy Nikkel (:tnikkel)]

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

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[Andrei Vaida [:avaida]]

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