1226238 - Remove "Share" and "Add to reading list" items from reader view toolbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[:Margaret Leibovic]

Both of these items are available in the main app menu, let's remove the redundant items in the reader view toolbar.

This will just leave us with the "Aa" text style button.

Anthony, how do you think we should style this single button?

Comment 1

[:Margaret Leibovic]

I started writing a patch for this and realized that removing the reading list button would lead to removing a ton of code.

Right now some of that code is shared with desktop, but the reading list is disabled by default on desktop?

Gijs, do you know if there is any reason to keep this around for desktop, or can I go ahead and remove this reading list support?

Comment 2

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #1)
> I started writing a patch for this and realized that removing the reading
> list button would lead to removing a ton of code.
> 
> Right now some of that code is shared with desktop, but the reading list is
> disabled by default on desktop?
> 
> Gijs, do you know if there is any reason to keep this around for desktop, or
> can I go ahead and remove this reading list support?

I believe it can be removed, but I'm not 100% sure. Pinging dolske for confirmation.

Note that pocket will still remain as a button on desktop, too (if the add-on is enabled), and there's infra for add-ons to add further buttons. Any restyling would have to take that into account. I don't know offhand if the same applies to mobile or not.

Comment 3

[Justin Dolske [:Dolske]]

The old pre-Pocket "Reading List" code can all be jettisoned.

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_controlsupdate_mob9.png

Since we only have 1 action left, it makes sense to do something similar to the FAB button. Here's what it'd look like inactive, on Mobile.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_controlsupdate_mob9_active.png

... Active, on mobile

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_tablet_controls.png

...Inactive, on Tablets. 

Button is 30px from the edges

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_tablet_controls_active.png

... and active on Tablets.

30 px from right edge, 15 px from bottom.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_mobcontrols.png

1) the fennec orange button is 56px and the 'x' is padded in from the edge by 20px 

2) Here's the change in padding for the active mode on phones.

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: spec_tabletcontrol.png

1) the 'x' is the same size, #AFB1B3

2) the 'x' is centered in the 56px col (same position)

3) the entire panel is the same but placed anchored to the new orange circle button (10px bottom margin on the orange button)

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Question: would it be possible to have the snackback UI push the new Reader view orange circle button (in comment 4) up? and then back down when the snack bar dismisses? [1]

[1]https://www.google.com/design/spec/components/snackbars-toasts.html#snackbars-toasts-usage

Comment 11

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ :D

Comment 12

[Andrzej Hunt :ahunt]

(In reply to Anthony Lam (:antlam) from comment #10)
> Question: would it be possible to have the snackback UI push the new Reader
> view orange circle button (in comment 4) up? and then back down when the
> snack bar dismisses? [1]
> 
> [1]https://www.google.com/design/spec/components/snackbars-toasts.
> html#snackbars-toasts-usage

Yes, it seems this should be possible (although I've never actually tried using this):

http://developer.android.com/reference/android/support/design/widget/FloatingActionButton.Behavior.html

We just need to make sure we use a CoordinatorLayout around the button, and everything else should hopefully be automatic - I guess we'll learn more once I'm actually implementing this!

Comment 13

[:Margaret Leibovic]

(In reply to Andrzej Hunt :ahunt from comment #12)
> (In reply to Anthony Lam (:antlam) from comment #10)
> > Question: would it be possible to have the snackback UI push the new Reader
> > view orange circle button (in comment 4) up? and then back down when the
> > snack bar dismisses? [1]
> > 
> > [1]https://www.google.com/design/spec/components/snackbars-toasts.
> > html#snackbars-toasts-usage
> 
> Yes, it seems this should be possible (although I've never actually tried
> using this):
> 
> http://developer.android.com/reference/android/support/design/widget/
> FloatingActionButton.Behavior.html
> 
> We just need to make sure we use a CoordinatorLayout around the button, and
> everything else should hopefully be automatic - I guess we'll learn more
> once I'm actually implementing this!

Be warned - the reader view controls are written in HTML/CSS, not native UI. We could take this opportunity to change that, but that would be a lot of work.

For this bug, I think we should keep the HTML/CSS controls, and just live with the fact that the snackbar is going to overlap it.

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_controlsupdate_mob9b_active.png

I'm not happy with the material looking expanded controls design I posted earlier. 

So I've decided to go with a simpler "active" state for this button for now.

Comment 16

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1226238 - (Part 1) Remove share button from reader view. r=ahunt,Gijs

Review commit: https://reviewboard.mozilla.org/r/35835/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35835/

Comment 17

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1226238 - (Part 2) Remove reader view footer. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/35837/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35837/

Comment 18

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1226238 - (Part 3) Remove reading list button from reader view. r=Gijs

Review commit: https://reviewboard.mozilla.org/r/35839/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35839/

Comment 19

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1226238 - (Part 4) Remove reading list toggle button from reader view. r=ahunt,Gijs

Review commit: https://reviewboard.mozilla.org/r/35841/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35841/

Comment 20

[:Margaret Leibovic]

I still need to write a final patch to update the toolbar style on mobile, but the patches I posted include all of the functional changes.

Gijs, it looks like there's some desktop reading list cruft in here that never got cleaned up. I tried looking for other desktop reading list code in the tree, but didn't see it, so hopefully it was already removed somewhere else.

Comment 21

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e3a8573e9e1

Comment 22

[:Gijs (he/him)]

Comment on attachment 8722038 [details]
MozReview Request: Bug 1226238 - (Part 1) Remove share button from reader view. r=ahunt,Gijs

https://reviewboard.mozilla.org/r/35835/#review32499

Comment 23

[:Gijs (he/him)]

Comment on attachment 8722039 [details]
MozReview Request: Bug 1226238 - (Part 2) Remove reader view footer. r=Gijs

https://reviewboard.mozilla.org/r/35837/#review32501

Comment 24

[:Gijs (he/him)]

Comment on attachment 8722040 [details]
MozReview Request: Bug 1226238 - (Part 3) Remove reading list button from reader view. r=Gijs

https://reviewboard.mozilla.org/r/35839/#review32503

Comment 25

[:Gijs (he/him)]

Comment on attachment 8722041 [details]
MozReview Request: Bug 1226238 - (Part 4) Remove reading list toggle button from reader view. r=ahunt,Gijs

https://reviewboard.mozilla.org/r/35841/#review32505

A bit confused about the visible attribute here, and all the scrolling code. Seems to have no effect on desktop. Is that just intentional? I don't see any sliding or whatever either. Seems like all that could be ripped out, too? The `_setToolbarVisibility` code actually never hides the toolbar right now, because of the check at the beginning of that method, so if we're showing it to begin with I think that means we can nuke all of that code completely... Withholding r+ for that for now...

::: browser/modules/ReaderParent.jsm
(Diff revision 1)
> -    "Reader:AddToList",
>      "Reader:ArticleGet",
>      "Reader:FaviconRequest",
> -    "Reader:ListStatusRequest",
> -    "Reader:RemoveFromList",
> -    "Reader:SystemUIVisibility",

Were these all just dead already? :-\

::: mobile/android/themes/core/aboutReaderControls.css:70
(Diff revision 1)
>  .toolbar[visible] {
>    bottom: 0;
>  }

Does this still do anything? Why do we need to add visible="true" to the toolbar? Removing it on desktop seems to have no effect...

Comment 26

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #25)
> Comment on attachment 8722041 [details]
> MozReview Request: Bug 1226238 - (Part 4) Remove reading list toggle button
> from reader view. r=ahunt,Gijs
> 
> https://reviewboard.mozilla.org/r/35841/#review32505
> 
> A bit confused about the visible attribute here, and all the scrolling code.
> Seems to have no effect on desktop. Is that just intentional? I don't see
> any sliding or whatever either. Seems like all that could be ripped out,
> too? The `_setToolbarVisibility` code actually never hides the toolbar right
> now, because of the check at the beginning of that method, so if we're
> showing it to begin with I think that means we can nuke all of that code
> completely... Withholding r+ for that for now...
> 
> ::: browser/modules/ReaderParent.jsm
> (Diff revision 1)
> > -    "Reader:AddToList",
> >      "Reader:ArticleGet",
> >      "Reader:FaviconRequest",
> > -    "Reader:ListStatusRequest",
> > -    "Reader:RemoveFromList",
> > -    "Reader:SystemUIVisibility",
> 
> Were these all just dead already? :-\

Yes.

> ::: mobile/android/themes/core/aboutReaderControls.css:70
> (Diff revision 1)
> >  .toolbar[visible] {
> >    bottom: 0;
> >  }
> 
> Does this still do anything? Why do we need to add visible="true" to the
> toolbar? Removing it on desktop seems to have no effect...

The toolbar transitions in and out from the bottom of the screen on mobile. This used to be dynamically set in the _requestReadingListStatus method, after we updated the reading list button, but I'm going to change the style of this button in my next patch. I just made enough changes to make the toolbar appear, so that I could test it wasn't broken, but I'm going to be changing these styles more in my next patch.

This is a case where I kinda wish we had forked versions of this for desktop/mobile. With the new mock-ups provided here, we're going to be moving away from a toolbar into a single font style button.

Comment 27

[:Margaret Leibovic]

I need a new asset of that "Aa" image that has white text instead of gray.

Comment 28

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_white_Aa.zip

try these

Comment 29

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1226238 - (Part 5) Update reader view toolbar styles. r=Gijs,ahunt

Review commit: https://reviewboard.mozilla.org/r/35905/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35905/

Comment 30

[:Margaret Leibovic]

I received IRL UX approval from antlam.

Comment 31

[:Gijs (he/him)]

Comment on attachment 8722209 [details]
MozReview Request: Bug 1226238 - (Part 5) Update reader view toolbar styles. r=Gijs,ahunt

https://reviewboard.mozilla.org/r/35905/#review32639

OK, but after this patch, we don't need the visible attribute on the toolbar anymore, do we? It doesn't seem to be used any longer...

Comment 32

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #31)
> Comment on attachment 8722209 [details]
> MozReview Request: Bug 1226238 - (Part 5) Update reader view toolbar styles.
> r=Gijs,ahunt
> 
> https://reviewboard.mozilla.org/r/35905/#review32639
> 
> OK, but after this patch, we don't need the visible attribute on the toolbar
> anymore, do we? It doesn't seem to be used any longer...

Sorry, yes, you're right. I'll remove that!

Comment 33

[Andrzej Hunt :ahunt]

Comment on attachment 8722038 [details]
MozReview Request: Bug 1226238 - (Part 1) Remove share button from reader view. r=ahunt,Gijs

https://reviewboard.mozilla.org/r/35835/#review33013

Comment 34

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=456ce825e85c

Comment 35

[Andrzej Hunt :ahunt]

Comment on attachment 8722041 [details]
MozReview Request: Bug 1226238 - (Part 4) Remove reading list toggle button from reader view. r=ahunt,Gijs

https://reviewboard.mozilla.org/r/35841/#review33253

Comment 36

[Andrzej Hunt :ahunt]

Comment on attachment 8722209 [details]
MozReview Request: Bug 1226238 - (Part 5) Update reader view toolbar styles. r=Gijs,ahunt

https://reviewboard.mozilla.org/r/35905/#review33255

Comment 37

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/e2414186e18ad96183afc486a4ecd3d0397ed04d
Bug 1226238 - (Part 1) Remove share button from reader view. r=ahunt,Gijs

https://hg.mozilla.org/integration/fx-team/rev/8ecfd8490dfc7705428e7d250778865595af5e63
Bug 1226238 - (Part 2) Remove reader view footer. r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/8c2597f8c188574c5d2ec6e37d9ca4d3676b4f3f
Bug 1226238 - (Part 3) Remove reading list button from reader view. r=Gijs

https://hg.mozilla.org/integration/fx-team/rev/89652aa76219bce7321e4452aa553699876f2817
Bug 1226238 - (Part 4) Remove reading list toggle button from reader view. r=ahunt,Gijs

https://hg.mozilla.org/integration/fx-team/rev/89eb5bb84d99324522bad7a707e08ff34932e6e9
Bug 1226238 - (Part 5) Update reader view toolbar styles. r=Gijs,ahunt

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e2414186e18a
https://hg.mozilla.org/mozilla-central/rev/8ecfd8490dfc
https://hg.mozilla.org/mozilla-central/rev/8c2597f8c188
https://hg.mozilla.org/mozilla-central/rev/89652aa76219
https://hg.mozilla.org/mozilla-central/rev/89eb5bb84d99

Comment 39

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2016-02-29 16:05:28.png

Verified as fixed using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 47.0a1 (2016-02-28)