Need feedback when an item is not removable from an area

VERIFIED FIXED in Firefox 29

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P3+])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
The URL bar / navigation widget will not be removable by default from the nav-bar. We currently don't do much to demonstrate this to the user, except to snap it back into place after they're done dragging.

We should indicate somehow that it can't be moved.
(Reporter)

Updated

5 years ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Created attachment 763323 [details] [diff] [review]
Patch

Hm, I don't know if we've figured out what a removable=false UI should look like. I put together a patch that fades and blurs out the customizable regions that are not potential targets for a customizable item.

To test this out, try dragging the location bar and you'll see that the customization palette and the menu panel contents will fade out and blur.

This effect is not applied if you move a removable=true item.
Attachment #763323 - Flags: review?(mconley)
Created attachment 763325 [details]
Screencast of patch

Stephen, what do you think about this effect?
Attachment #763325 - Flags: ui-review?(shorlander)
(Reporter)

Comment 3

5 years ago
Comment on attachment 763323 [details] [diff] [review]
Patch

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

Just a drive-by while we wait on shorlander feedback.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +709,5 @@
>      }
>  
> +    document.documentElement.removeAttribute("customizing-movingItem");
> +    let customizationBlockedElements = document.getElementsByClassName(kCustomizationBlocked);
> +    while (customizationBlockedElements.length) {

This block appears several times in this patch, which suggests to me that it'd probably be best to put it into its own function.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +39,5 @@
>    padding-right: 10px;
>  }
>  
> +.customization-blocked {
> +  position: relative;

Nit - put these in alphabetical order.
(In reply to Mike Conley (:mconley) from comment #3)
> > +.customization-blocked {
> > +  position: relative;
> 
> Nit - put these in alphabetical order.

Actually that position: relative; can be elided, it's just flotsam.
(Reporter)

Comment 5

5 years ago
Comment on attachment 763323 [details] [diff] [review]
Patch

According to dolske and shorlander, this isn't the effect we want. Instead, we want to try to "trap" the non-removable item inside its container, much the way we trap tabs inside the tabstrip when dragging them.
Attachment #763323 - Flags: review?(mconley)
Attachment #763325 - Flags: ui-review?(shorlander)
(Reporter)

Comment 6

5 years ago
These bugs didn't make it into the UR Build that went out in bug 879846. Clearing the [User Research Build+] flag.
Whiteboard: [Australis:M7][User Research Build+] → [Australis:M7]
(Reporter)

Comment 7

5 years ago
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?] [Australis:P3]
Whiteboard: [Australis:M?] [Australis:P3] → [Australis:M?][Australis:P3]
Created attachment 821073 [details] [diff] [review]
WIP patch (doesn't work yet)

Checkpointing my work here. The basic idea is to get away from using the dragImage and to make the dragged element position:absolute and change the top/left of the element while dragging. We can then clamp the values of top/left to the customizable area that it is home to if it is removable=false.
Attachment #763323 - Attachment is obsolete: true
Right now when one of the palette items is set as position:absolute, it ends up getting rendered as though it is position:static/display:block, creating its own block and moving other palette items to separate rows, also not moving when top/left are adjusted. I need to figure this out before proceeding further with this bug.
Duplicate of this bug: 936093
(Assignee)

Updated

5 years ago
Duplicate of this bug: 941042
Assignee: jaws → nobody
Status: ASSIGNED → NEW
(Reporter)

Comment 12

5 years ago
We've got a tentative plan to grey out non-removable items and not allow dragging of them.

phlsa is going to work on a mock-up.
Assignee: nobody → philipp
(Assignee)

Updated

5 years ago
Depends on: 931108
Created attachment 8361108 [details]
Mockup

Mockup is attached.
While we are at it, we can also implement Stephens suggestions for shrinking the location bar to make space for new items and that seems to be related.
The opacity of the location bar in the Mockup is 40%.
After discussing this with Philipp, the spec is formalized a bit more:

Additional space is needed at the start of a non-movable widget when it's the first widget inside the cust. area, to indicate clearly that widgets may be placed before it.
The width of the space needs to be roughly the same as a standard button widgets' width.
(In reply to Mike Conley (:mconley) from comment #12)
> We've got a tentative plan to grey out non-removable items and not allow
> dragging of them.

What's the plan for the menu button? The mockup doesn't grey it out, although it's not removable, in fact not even movable.
(In reply to Peter Retzer (:pretzer) from comment #15)
> (In reply to Mike Conley (:mconley) from comment #12)
> > We've got a tentative plan to grey out non-removable items and not allow
> > dragging of them.
> 
> What's the plan for the menu button? The mockup doesn't grey it out,
> although it's not removable, in fact not even movable.

The menu button will not be movable, but the mockup probably didn't grey it out because it is shown in the "open" state.
(Reporter)

Updated

5 years ago
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M?][Australis:P3][investigate-fix-on-aurora]
(Assignee)

Comment 17

5 years ago
(In reply to Philipp Sackl [:phlsa] from comment #13)
> Created attachment 8361108 [details]
> Mockup
> 
> Mockup is attached.
> While we are at it, we can also implement Stephens suggestions for shrinking
> the location bar to make space for new items and that seems to be related.
> The opacity of the location bar in the Mockup is 40%.

I don't know how easy it is to keep the rest of the items right-aligned, but not the location bar, though. Additionally, what happens if the user customizes all the items to be to the left of the location bar?
(In reply to :Gijs Kruitbosch from comment #17)
> I don't know how easy it is to keep the rest of the items right-aligned, but
> not the location bar, though.

A temporary spacer placed in the customization-target would get the alignment as requested.

> Additionally, what happens if the user
> customizes all the items to be to the left of the location bar?

If nothing is on the right-side of the location bar, we can place a similar padding (compared to the empty left-side) on the right-side to show that it is possible to place an item to the right of the location bar.
(Assignee)

Comment 19

5 years ago
(In reply to Jared Wein [:jaws] from comment #18)
> (In reply to :Gijs Kruitbosch from comment #17)
> > I don't know how easy it is to keep the rest of the items right-aligned, but
> > not the location bar, though.
> 
> A temporary spacer placed in the customization-target would get the
> alignment as requested.

That's weird, though, because then dropped items will either hug the location bar or the other items, depending on where they're dropped. I'm not even entirely sure how our DnD code would deal with it.

> > Additionally, what happens if the user
> > customizes all the items to be to the left of the location bar?
> 
> If nothing is on the right-side of the location bar, we can place a similar
> padding (compared to the empty left-side) on the right-side to show that it
> is possible to place an item to the right of the location bar.

I guess I would just much prefer to implement this without the space between the location bar and the buttons. It's a lot fewer headaches, and I don't see a UX benefit to adding the space between the items.
(In reply to :Gijs Kruitbosch from comment #19)
> I guess I would just much prefer to implement this without the space between
> the location bar and the buttons. It's a lot fewer headaches, and I don't
> see a UX benefit to adding the space between the items.

That is exactly what we should do at this point. The shifting does have the benefit of making the customizable areas clearer, but that really should be a different bug.

So the actions here should be:
1) Reduce the opacity of the location bar in customization mode
2) Make it non-draggable
3) Don't show a hand cursor when hovering it

Comment 21

5 years ago
Note that the list-all tabs dropdown arrow is also not removable, but it can be moved inside the tabbar.
(In reply to Guillaume C. [:ge3k0s] from comment #21)
> Note that the list-all tabs dropdown arrow is also not removable, but it can
> be moved inside the tabbar.

Good point!
Given that the arrow is far less prominent though, I'd say that the current behavior is good enough here (it simply snaps back into place if you drag it outside the tab bar).
(Assignee)

Comment 23

5 years ago
Created attachment 8371424 [details] [diff] [review]
make non-removable items fade out and non-draggable and adjust their cursor for Australis,

This works. I think. Even for the dropmarker in the tabs toolbar.
Attachment #8371424 - Flags: review?(mdeboer)
(Assignee)

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs Kruitbosch from comment #23)
> Created attachment 8371424 [details] [diff] [review]
> make non-removable items fade out and non-draggable and adjust their cursor
> for Australis,
> 
> This works. I think. Even for the dropmarker in the tabs toolbar.

That's good to hear!
As for the drop marker: I think fixing it in place as you suggest is actually the right way to go. In case someone tries to move it, having a clear indication that this is not possible is better than the frustration of only having a few (unindicated) possible drop zones.
Comment on attachment 8371424 [details] [diff] [review]
make non-removable items fade out and non-draggable and adjust their cursor for Australis,

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

The JS part looks excellent! Only the styling bit needs a little more TLC...

::: browser/base/content/browser.css
@@ +249,5 @@
>    display: none;
>  }
>  
> +toolbarpaletteitem[removable="false"] {
> +  opacity: 0.5;

Why .5? We use .4 for disabled items AFAIK...

@@ +250,5 @@
>  }
>  
> +toolbarpaletteitem[removable="false"] {
> +  opacity: 0.5;
> +  cursor: default !important;

Why the !important here?
Attachment #8371424 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 26

5 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #25)
> Comment on attachment 8371424 [details] [diff] [review]
> make non-removable items fade out and non-draggable and adjust their cursor
> for Australis,
> 
> Review of attachment 8371424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The JS part looks excellent! Only the styling bit needs a little more TLC...
> 
> ::: browser/base/content/browser.css
> @@ +249,5 @@
> >    display: none;
> >  }
> >  
> > +toolbarpaletteitem[removable="false"] {
> > +  opacity: 0.5;
> 
> Why .5? We use .4 for disabled items AFAIK...

http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#1206

We use .5 on OSX > 10.6, so either of them doesn't match in all cases.

I don't think the 0.1 difference matters. Precious few people would even be able to eyeball the difference on things like the dropmarker icon.

However, if you prefer, I can make this a combination of ifdefs and media queries so we match Windows and 10.6 when they're used.

As for the cursor styling, because I wanted to avoid it being overridden by more specific styles for items inside the toolbarpaletteitem. However, it seems we don't do that for -moz-grabbing either.
Whiteboard: [Australis:M?][Australis:P3][investigate-fix-on-aurora] → [Australis:M?][Australis:P3+][investigate-fix-on-aurora]
(Assignee)

Comment 27

5 years ago
Comment on attachment 8371424 [details] [diff] [review]
make non-removable items fade out and non-draggable and adjust their cursor for Australis,

Per discussion on IRC, r=mike with the !important removed.

remote:   https://hg.mozilla.org/integration/fx-team/rev/68778cb67084
Attachment #8371424 - Flags: review- → review+
(Assignee)

Updated

5 years ago
Whiteboard: [Australis:M?][Australis:P3+][investigate-fix-on-aurora] → [Australis:P3+][fixed-on-fx-team]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 969486
(Assignee)

Comment 29

5 years ago
This broke the skipintoolbarset drag test. I landed a simple test fix:

remote:   https://hg.mozilla.org/integration/fx-team/rev/167f977b7b9c
https://hg.mozilla.org/mozilla-central/rev/68778cb67084
https://hg.mozilla.org/mozilla-central/rev/167f977b7b9c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(Assignee)

Comment 31

5 years ago
Created attachment 8372899 [details] [diff] [review]
Patch for Aurora
(Assignee)

Comment 32

5 years ago
Comment on attachment 8372899 [details] [diff] [review]
Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: less-than-optimal feedback trying to move non-removable items to different areas
Testing completed (on m-c, etc.): local, now on m-c
Risk to taking this patch (and alternatives if risky): low, mostly CSS changes. This patch effectively restores the behaviour that non-removable items had when customizing pre-Australis
String or IDL/UUID changes made by this patch: none
Attachment #8372899 - Flags: approval-mozilla-aurora?
Attachment #8372899 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fe51382be60
status-firefox29: --- → fixed
status-firefox30: --- → fixed
Whiteboard: [Australis:P3+][fixed-on-fx-team] → [Australis:P3+]
This has the unfortunate side effect of making the urlbar not movable at all. Before, you could take it and move it, say on the right-most location. Now, to do the same, you need to move every single other button. Should I file a followup bug?
(Assignee)

Comment 35

5 years ago
(In reply to Mike Hommey [:glandium] from comment #34)
> This has the unfortunate side effect of making the urlbar not movable at
> all. Before, you could take it and move it, say on the right-most location.
> Now, to do the same, you need to move every single other button. Should I
> file a followup bug?

This was the design decision made here. Short of backing out the entire fix, or going with a completely different fix, I don't see what could be done about it. Pretending you could move it anywhere and then snapping it back to the navbar was also bad UX - that's why we've done what we've done. You could file a followup bug to only allow dragging the item within the confines of the navbar, but that's very hard to fix, and won't be done for 29 (or probably even 30).
Depends on: 972266

Updated

5 years ago
QA Contact: cornel.ionce
I was able to confirm the fix for this issue on Windows 7 64-bit [1], Mac OS X 10.9.1 [2] and Ubuntu 13.10 64-bit [3], using:
- the latest Nightly (Build ID: 20140306030201)
- the latest Aurora (Build ID: 20140306004001)

Note: it's also worth mentioning that on Nightly, the Location Bar displays a dashed border at all times and the Title Bar displays it on hover, while Aurora does not display any kind of borders for its elements (all of this happens in customization mode only).

Please let me know if the note above does not depict an intended behavior.

[1] Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Firefox/30.0
[2] Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
[3] Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
You need to log in before you can comment on or make changes to this bug.