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

VERIFIED FIXED in Firefox 38

Status

()

Toolkit
Reader Mode
P3
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: phlsa, Assigned: bwinton, Mentored)

Tracking

Trunk
mozilla40
x86
All
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38+ verified, firefox39 fixed, firefox40 verified)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 1

3 years ago
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]

Comment 2

3 years ago
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 :)
Blocks: 558882
Mentor: margaret.leibovic

Updated

3 years ago
Duplicate of this bug: 1135553

Updated

3 years ago
Component: Reading List → Reader Mode
Product: Firefox → Toolkit
Whiteboard: [qx][diamond] → [qx][diamond][lang=js]
Version: Firefox 38 → Trunk
(Assignee)

Comment 4

3 years ago
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

3 years ago
(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

Comment 6

3 years ago
FYI, my work on bug 998031 should help with this, we should wait for that to land before working on this.
Duplicate of this bug: 1142307

Comment 8

3 years ago
(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
(Assignee)

Comment 10

3 years ago
Created attachment 8586977 [details] [diff] [review]
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…  :)
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+
(Assignee)

Comment 12

3 years ago
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 13

3 years ago
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-
(Assignee)

Comment 14

3 years ago
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
(Assignee)

Comment 15

3 years ago
Created attachment 8587390 [details] [diff] [review]
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.
Attachment #8586977 - Attachment is obsolete: true
Attachment #8587390 - Flags: ui-review?(philipp)
Attachment #8587390 - Flags: review?(margaret.leibovic)

Comment 16

3 years ago
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+
(Assignee)

Comment 18

3 years ago
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+
(Assignee)

Comment 19

3 years ago
[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
tracking-firefox38: --- → ?
Keywords: checkin-needed
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 21

3 years ago
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
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [qx][diamond][lang=js][fixed-in-fx-team] → [qx][diamond][lang=js]
Target Milestone: --- → mozilla40
status-firefox38: --- → affected
status-firefox39: --- → affected
tracking-firefox38: ? → +
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
status-firefox40: fixed → verified
Flags: needinfo?(bwinton)
(Assignee)

Comment 24

3 years ago
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.
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.
status-firefox38: fixed → verified
Removing qe-verify flag since this was already verified on Nightly 40 and Beta 38.
Flags: qe-verify+
(Assignee)

Updated

3 years ago
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.