1120735 - Implement desktop reader mode controls

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[:Margaret Leibovic]

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

Comment 1

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

Here are the design specs for the Desktop reader model controls: https://projects.invisionapp.com/share/XT21LURF8

Comment 2

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

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

Comment 3

[:Margaret Leibovic]

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.

Comment 4

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

created bug 1124647 as a followup to this bug for the reading list controls that won't be implemented here.

Comment 5

[Michael Maslaney [:mmaslaney] ([e-mail])]

I'll be providing Readermode control assets this week.

Comment 6

[:Margaret Leibovic]

Just a friendly reminder that I still need these assets :)

Comment 7

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: ReadingList-Readermode-Icons.zip

Let me know if you would also like them in PNG format?

Comment 8

[:Margaret Leibovic]

Attachment: WIP - Implement desktop reader mode controls

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)

Comment 9

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: ReadingList-Readermode-Icons.zip

Updated assets

Comment 10

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: ReadingList-Readermode-Icons.zip

Updated with new hover states

Comment 11

[:Margaret Leibovic]

Attachment: Implement desktop reader mode controls

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

Comment 12

[:Margaret Leibovic]

Attachment: screenshot

Comment 13

[Brian Nicholson (:bnicholson)]

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?

Comment 14

[:Margaret Leibovic]

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

Comment 15

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

(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 16

[Jared Wein [:jaws] (please needinfo? me)]

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)

Comment 17

[:Margaret Leibovic]

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

Comment 18

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/5ee524df8201

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ee524df8201

Comment 20

[Andrei Vaida [:avaida]]

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.