873518 - Remove the activity indicator and old cut/copy/paste buttons from the palette

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

The status indicator, or "throbber", is not going to be included in the list of widgets that are customizable.

Comment 1

[Dão Gottwald [:dao]]

You shouldn't have to migrate anything, customization code should gracefully handle IDs pointing to items that don't exist anymore.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #1)
> You shouldn't have to migrate anything, customization code should gracefully
> handle IDs pointing to items that don't exist anymore.

This.

Comment 3

[:Gijs (he/him)]

Attachment: Part 1: remove throbber

Comment 4

[:Gijs (he/him)]

Attachment: Part 2: Remove cut/copy/paste buttons

I haven't adapted the toolbar sprite image for this bug. I'm not sure if we'd consider the resulting churn worth it?

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 754752 [details] [diff] [review]
Part 1: remove throbber

>     else if (aStateFlags & nsIWebProgressListener.STATE_STOP) {
>       // This (thanks to the filter) is a network stop or the last
>-      // request stop outside of loading the document, stop throbbers
>-      // and progress bars and such
>+      // request stop outside of loading the document, stop
>+      // progress bars and such

Please leave this comment alone. The throbber example remains meaningful as to what kind of UI should be updated here. Whether we do or do not have a global throbber doesn't matter much for the sake of giving an example. Note that we no longer have a progress bar either.

Comment 6

[:Gijs (he/him)]

Attachment: Fix off-by-one when moving widgets

Found this trying to see if there were bits about our current code that break when the placements array contains non-existing widgets.

Comment 7

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

Comment on attachment 754752 [details] [diff] [review]
Part 1: remove throbber

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

r=me

As Dao mentioned, we can leave that comment unchanged.

::: browser/themes/osx/browser.css
@@ -2140,5 @@
> -  list-style-image: url("chrome://global/skin/icons/loading_16.png");
> -}
> -
> -#wrapper-navigator-throbber > #navigator-throbber {
> -  list-style-image: url("chrome://global/skin/icons/notloading_16.png");

It looks like notloading_16.png will be unused after this patch. We can probably just remove it as well.

Out of curiousity, what happened on osx when the throbber wasn't in the busy state? From the looks of it, it seems like no list-style-image would be supplied.

Comment 8

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #7)
> Comment on attachment 754752 [details] [diff] [review]
> Part 1: remove throbber
> 
> Review of attachment 754752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> As Dao mentioned, we can leave that comment unchanged.
> 
> ::: browser/themes/osx/browser.css
> @@ -2140,5 @@
> > -  list-style-image: url("chrome://global/skin/icons/loading_16.png");
> > -}
> > -
> > -#wrapper-navigator-throbber > #navigator-throbber {
> > -  list-style-image: url("chrome://global/skin/icons/notloading_16.png");
> 
> It looks like notloading_16.png will be unused after this patch. We can
> probably just remove it as well.

Huh, seems you're right. I checked for loading_16.png, that's still used in a couple of places, so I figured this would be the same.

> Out of curiousity, what happened on osx when the throbber wasn't in the busy
> state? From the looks of it, it seems like no list-style-image would be
> supplied.

Correct. (on 22b, at least)

Comment 9

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

Comment on attachment 754754 [details] [diff] [review]
Part 2: Remove cut/copy/paste buttons

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

We can update Toolbar.png later when we get images for the new widgets added to it.

::: browser/base/content/browser.js
@@ +3363,5 @@
>    // The UI is visible if the Edit menu is opening or open, if the context menu
>    // is open, or if the toolbar has been customized to include the Cut, Copy,
>    // or Paste toolbar buttons.
> +  // XXXgijs: this needs to be updated with the unified cut/copy/paste toolbarbutton
> +  // once that is customizable!

Please include a reference to bug 870901 here.

Comment 10

[:Gijs (he/him)]

All pushed to UX, notloading_16.png removed (also from jar.mn), throbber-referencing comment intact, bug 870901 mentioned, push URLs:

https://hg.mozilla.org/projects/ux/rev/4a5aa012dee7
https://hg.mozilla.org/projects/ux/rev/a17f84ab819e
https://hg.mozilla.org/projects/ux/rev/5b35c6c3e9e0

Comment 11

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/5b35c6c3e9e0
https://hg.mozilla.org/mozilla-central/rev/a17f84ab819e
https://hg.mozilla.org/mozilla-central/rev/4a5aa012dee7
https://hg.mozilla.org/mozilla-central/rev/798863d29f0c

Comment 12

[ganesh_sahai1]

Tested on :---
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:28.0) Gecko/20100101 Firefox/28.0
Version :   28.0a1
Platform Tested : "Windows 7 64 bit"

Status : Not completely fixed. Activity indicator is removed, but,not the  cut/copy/paste.

Comment 13

[Matthew N. [:MattN]]

(In reply to ganesh_sahai1 from comment #12)
> Status : Not completely fixed. Activity indicator is removed, but,not the 
> cut/copy/paste.

Hello, this bug is referring to individual cut, copy and paste buttons and you are likely referring to the unified cut/copy/paste set which are expected.

Comment 14

[ganesh_sahai1]

I am not seeing any cut, copy and paste buttons. Unified cut/copy/paste is there which as per the previous comment seems to be expected. So,given this state, this bug is fixed now.