1137211 - Font panel in reading view should disappear when clicking on the margins

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Currently, the font panel in reader view doesn't disappear when clicking on the margins/empty space of the page. It should consistently disappear whenever a location outside the panel itself is clicked.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

So, we could probably fix this by adding a click handler to the window at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm#790 but Margaret might know of a better way…

Comment 2

[:Margaret Leibovic]

We actually already have a similar click handler that's used to hide the toolbar on mobile:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm#201

I'm changing this a bit in bug 1120004, but I think we should have that click hide the popup on desktop, and toggle the controls on mobile.

Right now the reason this doesn't hide the controls on desktop is that we don't have a .toolbar-hidden rule in the desktop CSS, and that's what we use to hide the controls on mobile. So this is actually kinda hacky right now and could use some fixing :)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Margaret, since you seem to know a little more about how this should all work, would you mind mentoring this bug (or co-mentoring with me if that works out better for you)?

Comment 5

[:Margaret Leibovic]

(In reply to Blake Winton (:bwinton) from comment #4)
> Margaret, since you seem to know a little more about how this should all
> work, would you mind mentoring this bug (or co-mentoring with me if that
> works out better for you)?

Yes, sure! I already added myself in the mentor field :)

Comment 6

[:Margaret Leibovic]

FYI, my work on bug 998031 should help with this, we should wait for that to land before working on this.

Comment 8

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #6)
> FYI, my work on bug 998031 should help with this, we should wait for that to
> land before working on this.

Actually, I'm wrong, we don't need to wait for this.

Right now we have a click handler on the #container div:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm?force=1#52

And that will dismiss the popup. We used to have this click handler directly on the body, but that interfered with clicking on the toolbar controls on mobile. Perhaps we should try moving the click handler back to the body, but then check the target before deciding to dismiss the popup.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Just wanted to check in on this, any progress abhinav?

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: Does this work on Android?

I suspect we'll need to be a little more clever on line 214, but I'm interested in how this fails on Android…  :)

Comment 11

[Marco Mucci [:MarcoM]]

Hi Blake, can you provide a point value.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

I'm not sure whether it'll be one or two points until Margaret replies to the review request, but I promise to update it as soon as I know.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8586977 [details] [diff] [review]
Does this work on Android?

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

Oh! I remember why we needed this. When you click on the decorative "Aa" between the -/+ buttons, we will dismiss the popup. Does this happen on desktop, too? Sadly, I think we'll need to add some more logic to deal with clicks on non-buttons in the popup to make this do the right thing.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

We don't have a decorative "Aa" on desktop, so we don't run into this, but I'll add one for the purposes of debugging…  :)

Thanks for the review!

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch that should work on Android.

Okay, now we loop up the parent chain to see if we're in the popup, and only close the popup if we aren't.

I had a shortcut to see if we were in the container, but it only saved a couple of .parentNode and .id calls, so I thought the simplicity of the current approach would be better.

There is a little bit of oddness when clicking on the buttons in the footer and the toolbar, because they steal the click event, so we don't see it in this handler, and thus don't close the popup, but this patch definitely fixes the reported problem, so seems like an improvement.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8587390 [details] [diff] [review]
A patch that should work on Android.

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

Works on mobile. I just have a few code style nits.

::: toolkit/components/reader/AboutReader.jsm
@@ +210,5 @@
>        return;
>  
>      switch (aEvent.type) {
>        case "click":
> +        let target = aEvent.target;

This is something it looks like we fail to do elsewhere, but you should put braces around the case statement if you're defining a local variable, to avoid it leaking into other case statements and potentially causing problems.

@@ +214,5 @@
> +        let target = aEvent.target;
> +        while (target && target.id != "reader-popup")
> +          target = target.parentNode;
> +        if (!target)
> +          this._toggleToolbarVisibility();

Style nit: Brace single-line ifs.

Comment 17

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8587390 [details] [diff] [review]
A patch that should work on Android.

Looking good!

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8587390 [details] [diff] [review]
A patch that should work on Android.

Asking Jared for a review of the desktop portions, in case Margaret's review didn't cover those bits…

Comment 19

[Blake Winton (:bwinton) (:☕️)]

[Tracking Requested - why for this release]: The rest of the Reader Mode landed in 38, and it would be nice to have this there, too.

Uplift request:

[User impact if declined]: Users will be annoyed trying to dismiss the popup and not succeeding.
[Describe test coverage new/current, TBPL]: Manual testing, and hopefully some time baking on mozilla-central before we uplift it.
[Risks and why]: Fairly-low risk because its a reasonably-small change which impacts only reader mode.
[String/UUID change made/needed]: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/06855f1796ee

Comment 21

[:Margaret Leibovic]

Comment on attachment 8587390 [details] [diff] [review]
A patch that should work on Android.

(I think bwinton meant to flip these flags)

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06855f1796ee

Comment 23

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 40.0a1 (2015-04-05), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Please note that, although this specific scenario is fixed, there still are a few different ones where the font panel should be dismissed but in fact isn't:
* click the font panel → click the Reading List sidebar button 
* click the font panel → click inside the empty area of the Reading List sidebar
* click the font panel → select text and/or images from the page
* click the font panel → click the Add/Remove page from Reading List button from any location (location bar, reader view controls, reader view footer)

Blake, what's your take on these?

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Ideally I think they should also be fixed, but the patches for those would be much more invasive, and unlikely to be worth the risk for Firefox 38.  Did you want to file follow-up bugs for each of them?

(It's also sort of a tricky case, because the panel is in-content, and so clicks on the sidebar (which are part of the browser chrome) in theory _shouldn't_ change the page content, but it looks like it's part of the browser, so maybe it should…)

Comment 25

[Andrei Vaida [:avaida]]

(In reply to Blake Winton (:bwinton) from comment #24)
> Ideally I think they should also be fixed, but the patches for those would
> be much more invasive, and unlikely to be worth the risk for Firefox 38. 
> Did you want to file follow-up bugs for each of them?
> 
> (It's also sort of a tricky case, because the panel is in-content, and so
> clicks on the sidebar (which are part of the browser chrome) in theory
> _shouldn't_ change the page content, but it looks like it's part of the
> browser, so maybe it should…)

Sure, I'll file separate bugs for each of those - this way we'll be able to look further into each of them.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3998197ab5ec

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d69e01ecebe0

Comment 28

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Other issues found related to this scenarios are already filed and will be treated separately.

Comment 29

[Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify flag since this was already verified on Nightly 40 and Beta 38.