Closed Bug 1137211 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Reader Mode, defect, P3)

x86
All
defect
Points:
2

Tracking

()

VERIFIED FIXED
mozilla40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox38 + verified
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: phlsa, Assigned: bwinton, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qx:link:spec][diamond][lang=js])

Attachments

(1 file, 1 obsolete file)

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.
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…
Mentor: bwinton
Whiteboard: [qx] → [qx][diamond]
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 :)
Mentor: margaret.leibovic
Component: Reading List → Reader Mode
Product: Firefox → Toolkit
Whiteboard: [qx][diamond] → [qx][diamond][lang=js]
Version: Firefox 38 → Trunk
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)?
(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 :)
Priority: -- → P3
FYI, my work on bug 998031 should help with this, we should wait for that to land before working on this.
(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.
Assignee: nobody → abhinav.koppula
Mentor: jaws
Status: NEW → ASSIGNED
Flags: qe-verify+
QA Contact: andrei.vaida
Just wanted to check in on this, any progress abhinav?
Assignee: abhinav.koppula → nobody
Status: ASSIGNED → NEW
Attached patch Does this work on Android? (obsolete) — Splinter Review
I suspect we'll need to be a little more clever on line 214, but I'm interested in how this fails on Android…  :)
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #8586977 - Flags: review?(margaret.leibovic)
Hi Blake, can you provide a point value.
Blocks: 1132074
Iteration: --- → 40.1 - 13 Apr
Flags: needinfo?(bwinton)
Flags: firefox-backlog+
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.
Flags: needinfo?(bwinton)
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.
Attachment #8586977 - Flags: review?(margaret.leibovic) → review-
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!
Points: --- → 2
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.
Attachment #8586977 - Attachment is obsolete: true
Attachment #8587390 - Flags: ui-review?(philipp)
Attachment #8587390 - Flags: review?(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.
Attachment #8587390 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8587390 [details] [diff] [review]
A patch that should work on Android.

Looking good!
Attachment #8587390 - Flags: ui-review?(philipp) → ui-review+
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…
Attachment #8587390 - Flags: review?(jaws)
Attachment #8587390 - Flags: review?(jaws) → review+
[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
https://hg.mozilla.org/integration/fx-team/rev/06855f1796ee
Keywords: checkin-needed
Whiteboard: [qx][diamond][lang=js] → [qx][diamond][lang=js][fixed-in-fx-team]
Comment on attachment 8587390 [details] [diff] [review]
A patch that should work on Android.

(I think bwinton meant to flip these flags)
Attachment #8587390 - Flags: approval-mozilla-beta?
Attachment #8587390 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/06855f1796ee
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [qx][diamond][lang=js][fixed-in-fx-team] → [qx][diamond][lang=js]
Target Milestone: --- → mozilla40
Attachment #8587390 - Flags: approval-mozilla-beta?
Attachment #8587390 - Flags: approval-mozilla-beta+
Attachment #8587390 - Flags: approval-mozilla-aurora?
Attachment #8587390 - Flags: approval-mozilla-aurora+
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?
Status: RESOLVED → VERIFIED
Flags: needinfo?(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…)
Flags: needinfo?(bwinton)
(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.
Depends on: 1151732
Depends on: 1151734
Depends on: 1151735
Depends on: 1151737
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.
Removing qe-verify flag since this was already verified on Nightly 40 and Beta 38.
Flags: qe-verify+
Whiteboard: [qx][diamond][lang=js] → [qx:link:spec][diamond][lang=js]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: