915272 - Add Reader Mode Icon to the URL bar of Metro Firefox

Status: RESOLVED DUPLICATE of bug 927124

(Firefox for Metro Graveyard :: Theme, enhancement, P1)

Description

[Eklavya Mirani]

Meta Bug for project https://github.com/Yoric/Mozilla-Student-Projects/issues/57

The Reader mode button, as implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=558882, should enable the reader mode on a page when clicked.

The button is visible only when the page is supported by Readability for parsing, otherwise it remains hidden.

Comment 1

[Matt Brubeck (:mbrubeck)]

Yuan and Shorlander, please feel free to chime in if you want to give any UX/design feedback for this student project.  In general I think we can follow the lead of Firefox for Android, and Microsoft's design and typography guidelines.

Comment 2

[Eklavya Mirani]

Attachment: Added the reader mode icon to the left of the reload icon in urlbar

Changes:
Added Sprites for Reader mode icon.
Added Reader mode icon to the left of the reload button.
Clicking the Reader mode button logs a string message to the browser console.

There is just one issue, when the browser is snapped to the smallest size (in immersive mode), there is a lot of gap between the reader mode icon and the reload icon.

Comment 3

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

(In reply to eklavyamirani from comment #2)
> Created attachment 808538 [details] [diff] [review]
> Added the reader mode icon to the left of the reload icon in urlbar
> 
> Changes:
> Added Sprites for Reader mode icon.
> Added Reader mode icon to the left of the reload button.
> Clicking the Reader mode button logs a string message to the browser console.
> 
> There is just one issue, when the browser is snapped to the smallest size
> (in immersive mode), there is a lot of gap between the reader mode icon and
> the reload icon.

Hi eklavyamirani,
Could you please upload a screenshots of the reader mode icon and any different state you have? So I can make comments on your review request. Thanks!

Comment 4

[Eklavya Mirani]

Attachment: Screenshots for reader mode.

Hi Yuan,

As requested, here are the screenshots and the sprites that I created.
I am using Windows Server 2012, which allows me to snap the app to custom sizes, and in some sizes the urlbar becomes inconsistent as you can see in the screenshots. Time to create a new bug? :)

Thanks,
Eklavya

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 808538 [details] [diff] [review]
Added the reader mode icon to the left of the reload icon in urlbar

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

The code looks fine with some trivial changes (see below).  Thanks!  r=mbrubeck, but this is not ready to land until it's hooked up to the actual about:reader code.

>--- a/browser/metro/base/content/appbar.js

This part of the patch file is malformed.  Try running "hg qref -e" to get rid of it.

::: browser/metro/base/content/browser.xul
@@ +260,5 @@
>              <observes element="bcast_urlbarState" attribute="*"/>
>              <hbox id="toolbar-context-page" pack="end">
>                <circularprogressindicator id="download-progress"
>                                       oncommand="Appbar.onDownloadButton()"/>
> +			  

Minor nit: This patch contains some unnecessary whitespace changes.

Comment 6

[Eklavya Mirani]

Attachment: Fixed based on review comments.

Comment 7

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Comment on attachment 808730 [details]
Screenshots for reader mode.

The icon and the states look fine with me

Comment 8

[Sam Foster [:sfoster] (he/him)]

Patch is ready to land, but landing appears to be blocked by bug 795968 and other work owned by the desktop team.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #7)
> Comment on attachment 808730 [details]
> Screenshots for reader mode.
> 
> The icon and the states look fine with me

These upscaled toolbar buttons images look awful. I think we need to get ux to scale these up properly.

Comment 10

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

Jim, please find the updated reader mode spec here:


http://people.mozilla.org/~mmaslaney/readermode/index.html

Comment 11

[Matt Brubeck (:mbrubeck)]

This patch has been folded into bug 927124.