Closed
Bug 1137211
Opened 8 years ago
Closed 8 years ago
Font panel in reading view should disappear when clicking on the margins
Categories
(Toolkit :: Reader Mode, defect, P3)
Tracking
()
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)
3.05 KB,
patch
|
Margaret
:
review+
jaws
:
review+
phlsa
:
ui-review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•8 years ago
|
Whiteboard: [qx]
Assignee | ||
Comment 1•8 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•8 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: desktop-reader
Mentor: margaret.leibovic
Updated•8 years ago
|
Component: Reading List → Reader Mode
Product: Firefox → Toolkit
Whiteboard: [qx][diamond] → [qx][diamond][lang=js]
Version: Firefox 38 → Trunk
Assignee | ||
Comment 4•8 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•8 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 :)
Updated•8 years ago
|
Priority: -- → P3
Comment 6•8 years ago
|
||
FYI, my work on bug 998031 should help with this, we should wait for that to land before working on this.
Comment 8•8 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.
Updated•8 years ago
|
Assignee: nobody → abhinav.koppula
Mentor: jaws
Status: NEW → ASSIGNED
Updated•8 years ago
|
Flags: qe-verify+
QA Contact: andrei.vaida
Comment 9•8 years ago
|
||
Just wanted to check in on this, any progress abhinav?
Updated•8 years ago
|
Assignee: abhinav.koppula → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
Hi Blake, can you provide a point value.
Assignee | ||
Comment 12•8 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•8 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•8 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•8 years ago
|
||
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•8 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+
Reporter | ||
Comment 17•8 years ago
|
||
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•8 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)
Updated•8 years ago
|
Attachment #8587390 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 19•8 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
Comment 20•8 years ago
|
||
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•8 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?
Comment 22•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/06855f1796ee
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [qx][diamond][lang=js][fixed-in-fx-team] → [qx][diamond][lang=js]
Target Milestone: --- → mozilla40
Updated•8 years ago
|
Updated•8 years ago
|
Attachment #8587390 -
Flags: approval-mozilla-beta?
Attachment #8587390 -
Flags: approval-mozilla-beta+
Attachment #8587390 -
Flags: approval-mozilla-aurora?
Attachment #8587390 -
Flags: approval-mozilla-aurora+
Comment 23•8 years ago
|
||
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?
Assignee | ||
Comment 24•8 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)
Comment 25•8 years ago
|
||
(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 28•8 years ago
|
||
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•8 years ago
|
||
Removing qe-verify flag since this was already verified on Nightly 40 and Beta 38.
Flags: qe-verify+
Assignee | ||
Updated•8 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.
Description
•