Closed Bug 1120735 Opened 5 years ago Closed 5 years ago

Implement desktop reader mode controls

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

Let's focus on the reader mode content styling in bug 1117258, and use this bug for styling the reader mode controls (e.g. font/color settings).

I think we should still keep the "add to reading list" and "share" controls hidden, as it will require more work to get the functionality for those working. (The reading list should probably just be part of the reading list work.)
OS: Android → All
Blocks: 1124011
Here are the design specs for the Desktop reader model controls: https://projects.invisionapp.com/share/XT21LURF8
Assignee: nobody → margaret.leibovic
(In reply to :Margaret Leibovic from comment #0)
> I think we should still keep the "add to reading list" and "share" controls
> hidden, as it will require more work to get the functionality for those
> working. (The reading list should probably just be part of the reading list
> work.)

Agreed on this and probably the open reading list / sidebar button as well.
I started playing around with this, and I realized I'm going to need image resources. In the meantime I just took some from Fennec, but some icons, like the "x" button and the left-pointing popup arrow, are specific to desktop.

For now I'm just re-using the same style popup logic that we have in place for Fennec, but maybe there's a more desktop-y way to do this? Maybe with a XUL panel? I don't know how that would work with this HTML page.
Flags: needinfo?(mmaslaney)
created bug 1124647 as a followup to this bug for the reading list controls that won't be implemented here.
I'll be providing Readermode control assets this week.
Flags: needinfo?(mmaslaney)
Just a friendly reminder that I still need these assets :)
Flags: needinfo?(mmaslaney)
Attached file ReadingList-Readermode-Icons.zip (obsolete) —
Let me know if you would also like them in PNG format?
Flags: needinfo?(mmaslaney)
Flags: firefox-backlog+
Blocks: 1130094
I feel like I've been working on this long enough that I should share a WIP.

My main challenge with this has been trying to deal with the fact that what we want to do on desktop is slightly different than what we do on Android, but I'm still trying to share the same HTML/JS.

At some point if I run into enough issues we should probably consider just forking the original Android code, but I don't think I'm at that point yet.

Outstanding issues to address here:
* Changing the serif/sans serif labels will change mobile, too. I need to check with antlam if we'd be okay with this, or I'll have to add some logic to diverge.
* The order of the buttons is wrong due to the fact that we're using the same mark-up as Android. Forking the HTML file may be the easiest way forward here to avoid annoying logic to have different button ordering.
* The font size buttons don't work... the Android code hard-codes the different font size values for various element types depending on the setting level, but I'd like to find a solution here that uses relative font sizes.
* Need a new image for the popup arrow.
* Hovering over corner buttons in the popup squares the corners of the popup. I think I may need to actually round one corner of those corner buttons :/

Issues to address in follow-up bugs:
* Share button doesn't work (bug 1130094)
* Add/remove reading list button doesn't work (I thought there was a bug for this but I can't find it)
Attachment #8560205 - Flags: feedback?(jaws)
Updated with new hover states
Attachment #8559336 - Attachment is obsolete: true
Attachment #8561497 - Attachment is obsolete: true
This patch ended up being kind of large... I had to make some Android modifications in order to keep sharing the core logic, but this manages to implement the desktop controls as specified without changing Android at all.

I can work with mmaslaney to refine polish details, but I think this is a large enough patch that it would be nice to land, then iterate.

A few issues we'll need to work out:
* Toolbar overlaps content if window becomes too narrow.
* Light/Dark/Sepia controls are a bit confusing, since they don't have an active/selected state.
* General polish details like padding and the shadow around the popup.
* Hooking up the reading list buttons
Attachment #8560205 - Attachment is obsolete: true
Attachment #8560205 - Flags: feedback?(jaws)
Attachment #8562393 - Flags: review?(jaws)
Attachment #8562393 - Flags: review?(bnicholson)
Attached image screenshot
Comment on attachment 8562393 [details] [diff] [review]
Implement desktop reader mode controls

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

::: toolkit/components/reader/AboutReader.jsm
@@ +159,5 @@
> +  get _isToolbarVertical() {
> +    if (this._toolbarVertical !== undefined) {
> +      return this._toolbarVertical;
> +    }
> +    return this._toolbarVertical = Services.prefs.getBoolPref("reader.toolbar.vertical");

You could drop the extra var/condition by replacing the getter:

delete this._isToolbarVertical;
this._isToolbarVertical = Services.prefs...;

@@ +253,5 @@
>      this._mm.sendAsyncMessage("Reader:ListStatusRequest", { url: this._article.url });
>    },
>  
> +  _onReaderClose: function Reader_onToggle() {
> +    this._win.location.href = this._getOriginalUrl();

I would have expected a close button to pop the reader URL on the back/forward stack, so hitting the browser back button after closing reader mode would take the user to the previous page. If we treat close as a new location, that means opening and closing reader mode will push new history entries on the stack each time, and clicking the back button after closing reader mode would put them back into reader mode.

But we also can't just say browser.goBack() since the user may have gone directly to an about:reader URL from the reading list. UX call I guess.

::: toolkit/themes/windows/global/aboutReader.css
@@ +9,4 @@
>    margin-right: auto;
>  }
>  
>  .light {

Missing .light-button?
Attachment #8562393 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #13)

> @@ +253,5 @@
> >      this._mm.sendAsyncMessage("Reader:ListStatusRequest", { url: this._article.url });
> >    },
> >  
> > +  _onReaderClose: function Reader_onToggle() {
> > +    this._win.location.href = this._getOriginalUrl();
> 
> I would have expected a close button to pop the reader URL on the
> back/forward stack, so hitting the browser back button after closing reader
> mode would take the user to the previous page. If we treat close as a new
> location, that means opening and closing reader mode will push new history
> entries on the stack each time, and clicking the back button after closing
> reader mode would put them back into reader mode.
> 
> But we also can't just say browser.goBack() since the user may have gone
> directly to an about:reader URL from the reading list. UX call I guess.

Yeah, that's an issue we dealt with on Fennec. For that reason, we decided that the "exit reader mode" button would just load the original URL, rather than navigate back. But yeah, maybe we should add a flag or something to help us make a better decision here.

This actually starts to approach the bigger issue of reader mode right now just loading another page, instead of being some sort of overlay, like what Safari does. But fixing that would be a much bigger task.
(In reply to :Margaret Leibovic from comment #11)
> I can work with mmaslaney to refine polish details, but I think this is a
> large enough patch that it would be nice to land, then iterate.
> 
> A few issues we'll need to work out:
> * Toolbar overlaps content if window becomes too narrow.
> * Light/Dark/Sepia controls are a bit confusing, since they don't have an
> active/selected state.

Just thinking out loud here, could we do a preview of these themes on hover?  Just an idea, I'll leave it up to @mmaslaney.

> * General polish details like padding and the shadow around the popup.
> * Hooking up the reading list buttons
Comment on attachment 8562393 [details] [diff] [review]
Implement desktop reader mode controls

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

The 'selected' orange bottom border isn't applied to the font-size or the color-mode of the document. In particular, due to the dark color of the "Dark" option, I got confused a couple times thinking that "Dark" was selected but not being applied due to a bug. See http://screencast.com/t/gKWcxLo3cB for a screenshot. (Disregard the wrong fonts being used in the screenshot). Since this is not enabled by default, I'm not sure if we should land this with or without changing how we style the "Dark" option first.

Another thing to note in that screenshot is the large size of the close button. Is it supposed to be that tall?

Also, we should get a follow-up bug on file to use better fonts than 'serif' and 'sans-serif'. Those are particularly ugly fonts and it hurts the reader mode experience.

::: toolkit/themes/shared/reader/RM-Add-24x24.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- Generator: Adobe Illustrator 18.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->

Can you clean up these SVG files to remove this extra cruft and also fix the indentation? (the Generator comment and the id="Icons" in this file)
Attachment #8562393 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Comment on attachment 8562393 [details] [diff] [review]
> Implement desktop reader mode controls
> 
> Review of attachment 8562393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The 'selected' orange bottom border isn't applied to the font-size or the
> color-mode of the document. In particular, due to the dark color of the
> "Dark" option, I got confused a couple times thinking that "Dark" was
> selected but not being applied due to a bug. See
> http://screencast.com/t/gKWcxLo3cB for a screenshot. (Disregard the wrong
> fonts being used in the screenshot). Since this is not enabled by default,
> I'm not sure if we should land this with or without changing how we style
> the "Dark" option first.

Yeah, I think we need a follow-up bug to refine the selected/active/hover states for the popup items. I'll work with mmaslaney on that this afternoon.

> Another thing to note in that screenshot is the large size of the close
> button. Is it supposed to be that tall?

Yes, that's what's in the mock-up:
https://projects.invisionapp.com/share/XT21LURF8#/screens

> Also, we should get a follow-up bug on file to use better fonts than 'serif'
> and 'sans-serif'. Those are particularly ugly fonts and it hurts the reader
> mode experience.

Yeah, already have bug 1120518 on file, and there was a firefox-dev thread about this:
https://mail.mozilla.org/pipermail/firefox-dev/2015-January/002629.html

I've been punting on this for the time being, but we should get around to working on this when the bigger pieces are in place.

> ::: toolkit/themes/shared/reader/RM-Add-24x24.svg
> @@ +1,2 @@
> > +<?xml version="1.0" encoding="utf-8"?>
> > +<!-- Generator: Adobe Illustrator 18.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
> 
> Can you clean up these SVG files to remove this extra cruft and also fix the
> indentation? (the Generator comment and the id="Icons" in this file)

Sure thing.
Blocks: 1132656
Blocks: 1132659
Blocks: 1132665
https://hg.mozilla.org/mozilla-central/rev/5ee524df8201
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify?
QA Contact: andrei.vaida
Verified fixed on Nightly 39.0a1 (2015-02-25) and Aurora 38.0a2 (2015-02-25), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5. 

Filed separate bugs for a few inconsistencies I found.
Status: RESOLVED → VERIFIED
Flags: qe-verify? → qe-verify+
No longer depends on: 1136716
You need to log in before you can comment on or make changes to this bug.