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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks 1 bug)

35 Branch
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 verified)

Details

Attachments

(12 attachments, 2 obsolete attachments)

217.32 KB, image/png
Details
226.03 KB, image/png
Details
238.06 KB, image/png
Details
49.29 KB, image/png
Details
227.25 KB, image/png
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
Gijs
: review+
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Details
4.29 KB, application/zip
Details
58 bytes, text/x-review-board-request
ahunt
: review+
Gijs
: review+
Details
322.40 KB, image/png
Details
(Assignee)

Description

4 years ago
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?
Flags: needinfo?(alam)
(Assignee)

Updated

4 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Updated

3 years ago
Blocks: migrate-RL
(Assignee)

Comment 1

3 years ago
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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 2

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dolske)
The old pre-Pocket "Reading List" code can all be jettisoned.
Flags: needinfo?(dolske)
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.
Flags: needinfo?(alam)
Posted image prev_controlsupdate_mob9_active.png (obsolete) —
... Active, on mobile
...Inactive, on Tablets. 

Button is 30px from the edges
... and active on Tablets.

30 px from right edge, 15 px from bottom.
Posted image spec_mobcontrols.png (obsolete) —
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.
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)
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
^ :D
Flags: needinfo?(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!
Flags: needinfo?(ahunt)
(Assignee)

Comment 13

3 years ago
(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.
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.
Attachment #8709693 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1249433
Attachment #8709712 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1249433
(Assignee)

Comment 20

3 years ago
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.

Updated

3 years ago
Attachment #8722038 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 22

3 years ago
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

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

https://reviewboard.mozilla.org/r/35837/#review32501
Attachment #8722039 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

3 years ago
Attachment #8722040 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 24

3 years ago
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

3 years ago
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...
Attachment #8722041 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 26

3 years ago
(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.
(Assignee)

Comment 27

3 years ago
I need a new asset of that "Aa" image that has white text instead of gray.
Flags: needinfo?(alam)
Posted file icon_white_Aa.zip
try these
Flags: needinfo?(alam) → needinfo?(margaret.leibovic)
(Assignee)

Comment 30

3 years ago
I received IRL UX approval from antlam.
Flags: needinfo?(margaret.leibovic)

Updated

3 years ago
Attachment #8722209 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 31

3 years ago
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...
(Assignee)

Comment 32

3 years ago
(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 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
Attachment #8722038 - Flags: review?(ahunt) → review+
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
Attachment #8722041 - Flags: review?(ahunt) → review+
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
Attachment #8722209 - Flags: review?(ahunt) → review+
Verified as fixed using:
Device: One A2001 (Android 5.1.1) 
Build: Firefox for Android 47.0a1 (2016-02-28)
(Assignee)

Updated

3 years ago
Depends on: 1252465
(Assignee)

Updated

3 years ago
Depends on: 1258455
You need to log in before you can comment on or make changes to this bug.