963576 - Dotted outline doesn't appear in customizable toolbars anymore

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Matthew N. [:MattN]]

UX Branch range for the outline on the nav-bar:
Last good revision: 176f2644c521 (2013-11-06)
First bad revision: 88104a2deda5 (2013-11-07)
Pushlog:
http://hg.mozilla.org/projects/ux/pushloghtml?fromchange=176f2644c521&tochange=88104a2deda5

Likely caused by bug 932963:
http://hg.mozilla.org/projects/ux/diff/9f6d6fa60cf0/browser/themes/shared/customizableui/customizeMode.inc.css

customize-entered is never set on the nav-bar

Shouldn't all customizable areas have the outline or did UX decide to only to the nav-bar at some point?

http://hg.mozilla.org/projects/ux/diff/fcf3103a14f8/browser/themes/shared/customizableui/customizeMode.inc.css (bug 930045) made this not the case but it's not clear that was intentional.

We should have a test to check that the outline is present on the areas where we expect.

Comment 1

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Drop area indicators

Here's a quick mockup of the drop areas that should be indicated while dragging. It is not pixel perfect – please use the styling of the borders that is already being used in the panel.

Comment 2

[Matthew N. [:MattN]]

We're going to just outline the whole customizable area for now as implement attachment 8365105 [details] requires bigger changes.

Comment 3

[u428464]

(In reply to Philipp Sackl [:phlsa] from comment #1)
> Created attachment 8365105 [details]
> Drop area indicators
> 
> Here's a quick mockup of the drop areas that should be indicated while
> dragging. It is not pixel perfect – please use the styling of the borders
> that is already being used in the panel.

What about the bookmarks toolbar ?

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Guillaume C. [:ge3k0s] from comment #3)
> What about the bookmarks toolbar ?

Let's also have the outline there, around the entire bookmarks bar (just as with the nav bar).

Comment 5

[u428464]

Technically the menu bar (if enabled) is also a customization area.

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Highlighted drop areas

OK, seeing the implications of that issue and having discussed it a little further, I think we should actually go back one step.

Let's:
1) Highlight only the toolbar and the panel areas
2) Keep that highlight persistent (not just while dragging items)

The reason is that the intention of the highlighted areas is to show people where it makes most sense to put their stuff. While you can place items in the menu bar, tab strip and bookmarks bar, those are not the places that make most sense for most people. We shouldn't keep anyone from dragging stuff there, but we don't have to encourage it either.

Comment 7

[Matthew N. [:MattN]]

(In reply to Philipp Sackl [:phlsa] from comment #6)
> We shouldn't keep anyone from dragging stuff there, but we don't have to encourage it either.

There's going to be an implication that they can't drag it elsewhere and IMO that's worse than no outlines.

What about adding the outline on other toolbars upon dragover of them? At least it's somewhat more discoverable?

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Matthew N. [:MattN] from comment #7)
> What about adding the outline on other toolbars upon dragover of them? At
> least it's somewhat more discoverable?

I think that is an excellent idea!
It solves the issue of having too many indicators visible at once, yet gives feedback.
I'd even go one step further and say that we could just highlight the areas on normal hover. That would make them even more discoverable and also encourage picking up items that are already in the UI.

Comment 9

[Matthew N. [:MattN]]

Attachment: WIP 1 - Outline nav-bar and placeholders by default and other customizable areas on dragOver/hover

There is still quite a bit to do and hover styling interacts poorly with onDrag styling since the hover style can continue apply after a drop until the cursor moves. It's not major but not polished.

I couldn't do as much of this with CSS alone since :-moz-drag-over only matches when the target is the dropzone, not also for the equivalent of the currentTarget. This means drag over the toolbar may work but if you drag over another toolbar button, the selector no long matches. This seems to be intentional in the code as only :hover and :active are treated as "hierarchical"[1].

The JS needs to be finished to make sure I didn't miss cases, optimized for performance, and cleaned up.

I'll need some UI feedback on the outline-offset and outline-color (and possibly margins) eventually.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/events/nsEventStateManager.cpp?mark=5031,5076-5077#5015

Comment 10

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I know this is WIP, but here's a couple of things I found. You already mentioned some them in your comment:

- The bottom border of the border around the main toolbar should be 1px higher up (http://cl.ly/image/083r0H0q132t)
- Same goes for the bottom border around the bookmarks bar
- Left and right border should be inside the toolbar (http://cl.ly/image/0L2A0Y2K2C1K)
- Would it be possible to have the border around the tab bar only on the right side of the tabs? You can't put anything in front of the tabs anyway and it creates a weird collision with the tabs.
- Styling-wise, I think we should go with what is in the mockup. Here's an annotated version: http://cl.ly/image/1Q0W1v2B0A0M
- When we use the lighter background color as in the mockup, maybe it makes sense to have a quick transition when hovering the highlighted areas.
- During the transition into customization mode, there is a border around the entire window. That shouldn't be there.

Comment 11

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #10)
> I know this is WIP, but here's a couple of things I found. You already
> mentioned some them in your comment:
> 
> - The bottom border of the border around the main toolbar should be 1px
> higher up (http://cl.ly/image/083r0H0q132t)
> - Same goes for the bottom border around the bookmarks bar
> - Left and right border should be inside the toolbar
> (http://cl.ly/image/0L2A0Y2K2C1K)
> - Would it be possible to have the border around the tab bar only on the
> right side of the tabs? You can't put anything in front of the tabs anyway
> and it creates a weird collision with the tabs.

Yes you can?

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #11)
> > - Would it be possible to have the border around the tab bar only on the
> > right side of the tabs? You can't put anything in front of the tabs anyway
> > and it creates a weird collision with the tabs.
> 
> Yes you can?

Ah, you're right! Evidently it is just extremely hard to do on OS X (you actually need to drop the item ON the tabs; it doesn't work if you drop them on the empty space in front of them.

So here's another illustration of how we could tackle that situation: http://is.gd/rlMz12

Comment 13

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Sorry, the illustration in comment #12 was cropped wrongly. Here's the actual version: http://cl.ly/image/2O3E0C3M2431

Comment 14

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Realizing that the image in my last comment could be quite misleading, here's a better version. Note: it's about the tab strip, not the nav bar :)
http://cl.ly/image/3H0y0e3n1e0i

Comment 15

[Matthew N. [:MattN]]

Attachment: v.1 Provide attributes/classes to style customization targets

After a lot of struggling, I believe this provides the plumbing to achieve the effects we want. The focus of this patch isn't on the visuals but instead of providing attributes and classes so we can use CSS rules to implement the desired visuals in a separate patch. I'm tempted to get this landed as-is and do the styling in a separate bug (since it's also non-trivial) but I'm not sure if the outline overlapping some borders is considered too ugly.

Comment 16

[Matthew N. [:MattN]]

Attachment: Part 2 - WIP.1 - Switch to pseudo-elements for the outline for positioning flexibility

The only way I could come up with to avoid overlapping the left/right borders and to tweak the vertical position of some of the outlines was to switch to pseudo-elements that can then be positioned wherever they are needed. If outline-offset supported four-value syntax (top, right, bottom, left) then this may not be necessary but I don't think we could expect that in 29 (or possibly ever).

I think only showing the outline at the ends of the tabs toolbar isn't possible with just CSS if you want it to outline buttons that are placed in those positions too (as opposed to just creating empty placeholders before the tabstrip). I think this should be handled in a follow-up if we can't live with outlining the whole customizable target like we do with everything else.

I haven't actually adjusted the outline positions yet because I wanted to make sure there wasn't objections to the approach but the horizontal border overlap issues were fixed for free as part of this.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8379621 [details] [diff] [review]
Part 2 - WIP.1 - Switch to pseudo-elements for the outline for positioning flexibility

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

This is terrible, but I don't see a better way of doing this.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +26,5 @@
>  }
>  
> +#main-window .customization-target:not(#PanelUI-contents)::before {
> +  /* Prevent jumping of tabs when switching a window between inactive and active (bug 853415). */
> +  -moz-box-ordinal-group: 0;

This scares me, also wrt bug 962803 and bug 974819.

Can we at least only add the pseudoelement within customize mode?

I'd prefer to not have a .2s transition over messing with our layout outside of customize mode this late in the 29 cycle.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8379568 [details] [diff] [review]
v.1 Provide attributes/classes to style customization targets

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

As noted in the feedback on the other patch, I think I'd prefer this to be restricted to customize mode and not have the transitions, also to avoid impacting the perf work that we're doing to make transitioning into customize mode smooth.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1787,5 @@
> +   * outlines to indicate that an area is customizable.
> +   *
> +   * @param aWindow                The XUL window in which outlines should be updated.
> +   * @param {Element} [aArea=null] The element of the customizable area to add the outline to.
> +   *                               If aArea is falsy, the outline will be removed from all areas.

These doc-comments lie a little. s/all areas/all toolbar areas/ (and likewise throughout - also note that you don't ever set the attribute on non-toolbar notes even if they're passed as the argument; perhaps just make the argument aToolbarArea ?)

@@ +1796,5 @@
> +      if (CustomizableUI.getAreaType(area) != CustomizableUI.TYPE_TOOLBAR) {
> +        continue;
> +      }
> +      let target = CustomizableUI.getCustomizeTargetForArea(area, aWindow);
> +      target.removeAttribute("customizing-dragOver");

nit: we normally don't use camelcase for attributes, and indeed already have a 'dragover' attribute for the individual nodes. Maybe 'dragoverarea' instead?

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +21,5 @@
> +#PanelUI-contents > .panel-customization-placeholder {
> +  -moz-outline-radius: 2.5px;
> +  outline: 1px dashed rgba(102,102,102,0);
> +  transition-duration: 0.2s;
> +  transition-property: outline-color;

For the non-navbar toolbars, this will set the transition property on that toolbar. However, those elements are transitioned as part of the customize mode transition, too. How do those two transitions interact, and doesn't this one override the other one (or vice-versa)?

What's more, can't the top selector be made specific to customize mode, so that these won't interfere with any other styling on the element (or cause reflows of some kind when the classes are set on the customization targets)?

Comment 19

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #17)
> Comment on attachment 8379621 [details] [diff] [review]
> Part 2 - WIP.1 - Switch to pseudo-elements for the outline for positioning
> flexibility
>
> > +#main-window .customization-target:not(#PanelUI-contents)::before {
> > +  /* Prevent jumping of tabs when switching a window between inactive and active (bug 853415). */
> > +  -moz-box-ordinal-group: 0;
> 
> This scares me, also wrt bug 962803 and bug 974819.

Do you have reason to believe the jumping is related to pseudo-elements? I didn't see that in those bugs.

> Can we at least only add the pseudoelement within customize mode?

I actually did this to help performance of the transition by changing less during it.

> I'd prefer to not have a .2s transition over messing with our layout outside
> of customize mode this late in the 29 cycle.

The transition I added wouldn't normally run during the transition. It's only upon hover or dragover so it would only matter if the user happened to be hovering a customizable toolbar at that time which is unlikely to be the case when entering the mode via the context menu or panel menu button.

(In reply to :Gijs Kruitbosch from comment #18)
> Comment on attachment 8379568 [details] [diff] [review]
> v.1 Provide attributes/classes to style customization targets
> 
> ::: browser/components/customizableui/src/CustomizeMode.jsm
> @@ +1787,5 @@
> > +   * outlines to indicate that an area is customizable.
> > +   *
> > +   * @param aWindow                The XUL window in which outlines should be updated.
> > +   * @param {Element} [aArea=null] The element of the customizable area to add the outline to.
> > +   *                               If aArea is falsy, the outline will be removed from all areas.
> 
> These doc-comments lie a little. s/all areas/all toolbar areas/ (and
> likewise throughout - also note that you don't ever set the attribute on
> non-toolbar notes even if they're passed as the argument; perhaps just make
> the argument aToolbarArea ?)

Okay, fixed. This is because I initially thought we allowed other areas to be registered but I saw that we only allow one panel area somewhere. I was going to also set the attribute on those other targets but have the CSS rules treat them differently by using placeholders instead for our panel menu.

> @@ +1796,5 @@
> > +      if (CustomizableUI.getAreaType(area) != CustomizableUI.TYPE_TOOLBAR) {
> > +        continue;
> > +      }
> > +      let target = CustomizableUI.getCustomizeTargetForArea(area, aWindow);
> > +      target.removeAttribute("customizing-dragOver");
> 
> nit: we normally don't use camelcase for attributes, and indeed already have
> a 'dragover' attribute for the individual nodes. Maybe 'dragoverarea'
> instead?

Okay, I used dragovertarget since it's technically the target, not necessarily the area.

> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> @@ +21,5 @@
> > +#PanelUI-contents > .panel-customization-placeholder {
> > +  -moz-outline-radius: 2.5px;
> > +  outline: 1px dashed rgba(102,102,102,0);
> > +  transition-duration: 0.2s;
> > +  transition-property: outline-color;
> 
> For the non-navbar toolbars, this will set the transition property on that
> toolbar. However, those elements are transitioned as part of the customize
> mode transition, too. How do those two transitions interact, and doesn't
> this one override the other one (or vice-versa)?

I didn't realize the margin moved to the toolbars. I'm looking at moving it back to one of the wrappers and so far CART is showing wins when I was just hoping for parity.

> What's more, can't the top selector be made specific to customize mode, so
> that these won't interfere with any other styling on the element (or cause
> reflows of some kind when the classes are set on the customization targets)?

"top" of what?

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

This looks MUCH better than before!

(In reply to Matthew N. [:MattN] from comment #16)
> I think only showing the outline at the ends of the tabs toolbar isn't
> possible with just CSS if you want it to outline buttons that are placed in
> those positions too (as opposed to just creating empty placeholders before
> the tabstrip). I think this should be handled in a follow-up if we can't
> live with outlining the whole customizable target like we do with everything
> else.

Not sure if I'm understanding you correctly there. If you look at http://cl.ly/image/3H0y0e3n1e0i you see that the indicator in the tab strip should not be restricted to the right side. It should, however, be a little shorter on the left side (see the pink indicator line).

Comment 21

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #20)
> This looks MUCH better than before!
> 
> (In reply to Matthew N. [:MattN] from comment #16)
> > I think only showing the outline at the ends of the tabs toolbar isn't
> > possible with just CSS if you want it to outline buttons that are placed in
> > those positions too (as opposed to just creating empty placeholders before
> > the tabstrip). I think this should be handled in a follow-up if we can't
> > live with outlining the whole customizable target like we do with everything
> > else.
> 
> Not sure if I'm understanding you correctly there. If you look at
> http://cl.ly/image/3H0y0e3n1e0i you see that the indicator in the tab strip
> should not be restricted to the right side. It should, however, be a little
> shorter on the left side (see the pink indicator line).

It might be easier to fix the code so you /can/ drop things in those places... Maybe. Not looked at this yet.

Comment 22

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #17)
> > Comment on attachment 8379621 [details] [diff] [review]
> > Part 2 - WIP.1 - Switch to pseudo-elements for the outline for positioning
> > flexibility
> >
> > > +#main-window .customization-target:not(#PanelUI-contents)::before {
> > > +  /* Prevent jumping of tabs when switching a window between inactive and active (bug 853415). */
> > > +  -moz-box-ordinal-group: 0;
> > 
> > This scares me, also wrt bug 962803 and bug 974819.
> 
> Do you have reason to believe the jumping is related to pseudo-elements? I
> didn't see that in those bugs.

bug 963512 certainly is, and this is just adding another one of these. I'd like to move away from pseudoelements in the tabbar; why don't we add real elements that we can use for this? I mean, I wrote 'this scares me', not "you won't get r+ with this in here", but if there's no other (good) way we can do this, then I guess this is what needs to happen.

> > Can we at least only add the pseudoelement within customize mode?
> 
> I actually did this to help performance of the transition by changing less
> during it.

Sure, I just figured we could add the pseudoelement after this? For instance, you could add it when the toolbar in question gets a customizing="true" attribute, with a transition-delay of 1s/500ms or something?

> > I'd prefer to not have a .2s transition over messing with our layout outside
> > of customize mode this late in the 29 cycle.
> 
> The transition I added wouldn't normally run during the transition.

*brain explodes*

> It's only upon hover or dragover so it would only matter if the user happened to
> be hovering a customizable toolbar at that time which is unlikely to be the
> case when entering the mode via the context menu or panel menu button.

You're right, I misread this. However, the transition styles still conflict with the ones on the toolbars already...

> (In reply to :Gijs Kruitbosch from comment #18)
> > Comment on attachment 8379568 [details] [diff] [review]
> > v.1 Provide attributes/classes to style customization targets
> > 
> > ::: browser/components/customizableui/src/CustomizeMode.jsm
> > @@ +1787,5 @@
> > > +   * outlines to indicate that an area is customizable.
> > > +   *
> > > +   * @param aWindow                The XUL window in which outlines should be updated.
> > > +   * @param {Element} [aArea=null] The element of the customizable area to add the outline to.
> > > +   *                               If aArea is falsy, the outline will be removed from all areas.
> > 
> > These doc-comments lie a little. s/all areas/all toolbar areas/ (and
> > likewise throughout - also note that you don't ever set the attribute on
> > non-toolbar notes even if they're passed as the argument; perhaps just make
> > the argument aToolbarArea ?)
> 
> Okay, fixed. This is because I initially thought we allowed other areas to
> be registered but I saw that we only allow one panel area somewhere. I was
> going to also set the attribute on those other targets but have the CSS
> rules treat them differently by using placeholders instead for our panel
> menu.

When you say 'fixed', did you mean to put up another patch?

> > ::: browser/themes/shared/customizableui/customizeMode.inc.css
> > @@ +21,5 @@
> > > +#PanelUI-contents > .panel-customization-placeholder {
> > > +  -moz-outline-radius: 2.5px;
> > > +  outline: 1px dashed rgba(102,102,102,0);
> > > +  transition-duration: 0.2s;
> > > +  transition-property: outline-color;
> > 
> > For the non-navbar toolbars, this will set the transition property on that
> > toolbar. However, those elements are transitioned as part of the customize
> > mode transition, too. How do those two transitions interact, and doesn't
> > this one override the other one (or vice-versa)?
> 
> I didn't realize the margin moved to the toolbars. I'm looking at moving it
> back to one of the wrappers and so far CART is showing wins when I was just
> hoping for parity.

This is confusing. Please discuss / get review/feedback from mconley regarding this.

> > What's more, can't the top selector be made specific to customize mode, so
> > that these won't interfere with any other styling on the element (or cause
> > reflows of some kind when the classes are set on the customization targets)?
> 
> "top" of what?

I meant the first of the two complete selectors used for that rule, id est

+#main-window .customization-target:not(#PanelUI-contents),

Comment 23

[Matthew N. [:MattN]]

(In reply to Philipp Sackl [:phlsa] from comment #20)
> Not sure if I'm understanding you correctly there. If you look at
> http://cl.ly/image/3H0y0e3n1e0i you see that the indicator in the tab strip
> should not be restricted to the right side. It should, however, be a little
> shorter on the left side (see the pink indicator line).

I understand the problem but I agree with Gijs that it may be easier to fix it so you can drop there. It's non-trivial to know the size of the gap on the left via CSS since it varies between platforms and OS version (10.6 is less than 10.7 IIRC). I think we should handle this issue in a separate bug as there is enough going on in my patches and my patch isn't introducing the issue, but it is highlighting it (technically outlining it :P).

Comment 24

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to Matthew N. [:MattN] from comment #19)
> > (In reply to :Gijs Kruitbosch from comment #17)
> > > Comment on attachment 8379621 [details] [diff] [review]
> > > Part 2 - WIP.1 - Switch to pseudo-elements for the outline for positioning
> > > flexibility
> > >
> > > > +#main-window .customization-target:not(#PanelUI-contents)::before {
> > > > +  /* Prevent jumping of tabs when switching a window between inactive and active (bug 853415). */
> > > > +  -moz-box-ordinal-group: 0;
> > > 
> > > This scares me, also wrt bug 962803 and bug 974819.
> > 
> > Do you have reason to believe the jumping is related to pseudo-elements? I
> > didn't see that in those bugs.
> 
> bug 963512 certainly is, and this is just adding another one of these. I'd
> like to move away from pseudoelements in the tabbar; why don't we add real
> elements that we can use for this? I mean, I wrote 'this scares me', not
> "you won't get r+ with this in here", but if there's no other (good) way we
> can do this, then I guess this is what needs to happen.

OK, that wasn't one of the two bugs you listed :P. Are you talking about real elements for this bug or that PB one? I don't think having outline elements always in the DOM is a good idea as they aren't normally needed. Dynamically creating them while entering customization mode may not be as bad but that creates a tighter coupling between presentation and content.

> > > Can we at least only add the pseudoelement within customize mode?
> > 
> > I actually did this to help performance of the transition by changing less
> > during it.
> 
> Sure, I just figured we could add the pseudoelement after this? For
> instance, you could add it when the toolbar in question gets a
> customizing="true" attribute, with a transition-delay of 1s/500ms or
> something?

Does adding a delay allow this to work when properties such as "display" are present?

> > > I'd prefer to not have a .2s transition over messing with our layout outside
> > > of customize mode this late in the 29 cycle.
> > 
> > The transition I added wouldn't normally run during the transition.
> 
> *brain explodes*

Perhaps I was wrong but I wasn't seeing that to be the case and I've since realized I had a misconception about CSS transitions (or perhaps the behaviour changed since they were prefixed).

> > It's only upon hover or dragover so it would only matter if the user happened to
> > be hovering a customizable toolbar at that time which is unlikely to be the
> > case when entering the mode via the context menu or panel menu button.
> 
> You're right, I misread this. However, the transition styles still conflict
> with the ones on the toolbars already...
>
> When you say 'fixed', did you mean to put up another patch?
Nope, I was still figuring out how to deal with the conflict above.

> > > ::: browser/themes/shared/customizableui/customizeMode.inc.css
> > I didn't realize the margin moved to the toolbars. I'm looking at moving it
> > back to one of the wrappers and so far CART is showing wins when I was just
> > hoping for parity.
> 
> This is confusing. Please discuss / get review/feedback from mconley
> regarding this.

I planned to discuss it with him in the dependency I will file.

> > > What's more, can't the top selector be made specific to customize mode, so
> > > that these won't interfere with any other styling on the element (or cause
> > > reflows of some kind when the classes are set on the customization targets)?
> > 
> > "top" of what?
> 
> I meant the first of the two complete selectors used for that rule, id est
> 
> +#main-window .customization-target:not(#PanelUI-contents),

Yes, I changed that based on your previous comment in my next version.

Comment 25

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Matthew N. [:MattN] from comment #23)
> I understand the problem but I agree with Gijs that it may be easier to fix
> it so you can drop there. It's non-trivial to know the size of the gap on
> the left via CSS since it varies between platforms and OS version (10.6 is
> less than 10.7 IIRC). I think we should handle this issue in a separate bug
> as there is enough going on in my patches and my patch isn't introducing the
> issue, but it is highlighting it (technically outlining it :P).

That approach is perfectly fine with me :)
I filed bug 976484 about it.

Comment 26

[Matthew N. [:MattN]]

Attachment: Part 1 - v.1.1 Provide attributes/classes to style customization targets

Comment 27

[:Gijs (he/him)]

Comment on attachment 8381260 [details] [diff] [review]
Part 1 - v.1.1 Provide attributes/classes to style customization targets

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

r=me with the below addressed/rebuffed

::: browser/base/content/browser.xul
@@ +624,5 @@
>               overflowbutton="nav-bar-overflow-button"
>               overflowtarget="widget-overflow-list"
>               context="toolbar-context-menu">
>  
> +      <hbox id="nav-bar-customization-target" flex="1">

We've been bitten by perf in the past when it came to attributes that weren't set in XUL but were added slightly later, so unless there is a compelling reason to have this behave differently, I would prefer to leave the class here.

Comment 28

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #27)
> We've been bitten by perf in the past when it came to attributes that
> weren't set in XUL but were added slightly later, so unless there is a
> compelling reason to have this behave differently, I would prefer to leave
> the class here.

I made the change for:
a) consistency as this was the only target that had the class
  and
b) to make it more clear that this attribute is now set at runtime rather than remaining static so developers wouldn't make assumptions about it e.g. do an MXR search and only find one hit in XUL and assume that's going to be the only match for a selector.

If I don't see a talos regression on try for tpaint/ts_paint/TART/CART/etc. then I'd rather leave it off. If there is a perf issue then I'd like to add it to all targets to address a).

Comment 29

[Matthew N. [:MattN]]

Attachment: Part 2 - v.1 Add an outline on entering customization mode or upon hover/drag over customization targets

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a84c86ec6682

Comment 30

[:Gijs (he/him)]

Comment on attachment 8381350 [details] [diff] [review]
Part 2 - v.1 Add an outline on entering customization mode or upon hover/drag over customization targets

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

Generally, this looks good.

Two main things:
- I think the outlines for the placeholders in the panel are too strong (at least on OS X). I don't need to see another patch for this, and I leave it up to you and UX to figure out if we want a lighter color there, or not, and/or if that should be dealt with in a followup bug (but keep in mind the uplift churn, please).
- You should run the customizeMode.inc.css bits by mconley. :-)

::: browser/themes/osx/browser.css
@@ +2576,5 @@
>  }
>  
>  @media (min-resolution: 2dppx) {
>    /* image preloading hack from shared/tabs.inc.css */
> +  #tabbrowser-tabs::before {

We keep moving this thing around. Is this really the best place?

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +33,5 @@
> +  outline-offset: -2px;
> +  pointer-events: none;
> +  position: absolute;
> +  top: 0;
> +  width: 100%;

I wonder if setting all of left/right/top/bottom to 0 would be easier/faster/correct-er for layout to understand, but meh.

@@ +55,5 @@
> +   placeholders instead. */
> +#main-window[customize-entered] .customization-target:not(#PanelUI-contents):hover::before,
> +#main-window[customize-entered] .customization-target[customizing-dragovertarget]:not(#PanelUI-contents)::before,
> +/* nav-bar and panel outlines are always shown */
> +/*#main-window[customize-entered] #nav-bar-customization-target.customization-target::before,*/

Please don't leave commented-out bits in, it's confusing.

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 8381350 [details] [diff] [review]
Part 2 - v.1 Add an outline on entering customization mode or upon hover/drag over customization targets

(In reply to :Gijs Kruitbosch from comment #30)
> Two main things:
> - I think the outlines for the placeholders in the panel are too strong (at
> least on OS X). I don't need to see another patch for this, and I leave it
> up to you and UX to figure out if we want a lighter color there, or not,
> and/or if that should be dealt with in a followup bug (but keep in mind the
> uplift churn, please).

Phillip, should I leave the panel outline the colour it was or what do you think?

> - You should run the customizeMode.inc.css bits by mconley. :-)

OK

> ::: browser/themes/osx/browser.css
> @@ +2576,5 @@
> >  }
> >  
> >  @media (min-resolution: 2dppx) {
> >    /* image preloading hack from shared/tabs.inc.css */
> > +  #tabbrowser-tabs::before {
> 
> We keep moving this thing around. Is this really the best place?

Well, as I said before, keep it near the tabs makes it easier to find. Nobody has had the time to check if it's even needed anymore.

> ::: browser/themes/shared/customizableui/customizeMode.inc.css
> @@ +33,5 @@
> > +  outline-offset: -2px;
> > +  pointer-events: none;
> > +  position: absolute;
> > +  top: 0;
> > +  width: 100%;
> 
> I wonder if setting all of left/right/top/bottom to 0 would be
> easier/faster/correct-er for layout to understand, but meh.
> 
> @@ +55,5 @@
> > +   placeholders instead. */
> > +#main-window[customize-entered] .customization-target:not(#PanelUI-contents):hover::before,
> > +#main-window[customize-entered] .customization-target[customizing-dragovertarget]:not(#PanelUI-contents)::before,
> > +/* nav-bar and panel outlines are always shown */
> > +/*#main-window[customize-entered] #nav-bar-customization-target.customization-target::before,*/
> 
> Please don't leave commented-out bits in, it's confusing.

That wasn't supposed to be commented out still.

Comment 32

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

Comment on attachment 8381350 [details] [diff] [review]
Part 2 - v.1 Add an outline on entering customization mode or upon hover/drag over customization targets

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

Assuming there's no significant dent in our CART numbers, I'd say this looks good to me.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +38,3 @@
>  }
>  
> +/* Shift the TabsToolbar outline up 2px since the #nav-bar is shitfted up by 1px and the

Typo - shitfted -> shifted.

@@ +43,5 @@
> +#main-window[customize-entered] #TabsToolbar.customization-target::before {
> +  top: -2px;
> +}
> +
> +/* The parents of the outline pseudo-elements need to be positioned to that the outline is positioned relative to it. */

position to -> position so.

Comment 33

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #31)
> (In reply to :Gijs Kruitbosch from comment #30)
> > Two main things:
> > - I think the outlines for the placeholders in the panel are too strong (at
> > least on OS X). I don't need to see another patch for this, and I leave it
> > up to you and UX to figure out if we want a lighter color there, or not,
> > and/or if that should be dealt with in a followup bug (but keep in mind the
> > uplift churn, please).
> 
> Phillip, should I leave the panel outline the colour it was or what do you
> think?

I ended up leaving the old outline colour on the panel placeholders for now.

Pushed:
https://hg.mozilla.org/integration/fx-team/rev/aa09dfcfce31
https://hg.mozilla.org/integration/fx-team/rev/0bfea5715ed4

Try: https://tbpl.mozilla.org/?tree=Try&rev=a84c86ec6682

Compare-talos: http://compare-talos.mattn.ca/?oldRevs=dfd35e3fabc2&newRev=a84c86ec6682&submit=true

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/aa09dfcfce31
https://hg.mozilla.org/mozilla-central/rev/0bfea5715ed4

Comment 35

[Tim Nguyen :ntim]

Unfortunatly, I can't see the outline on dragover (with an item grabbed). But, I can see the outline on hover.

Comment 36

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #35)
> Unfortunatly, I can't see the outline on dragover (with an item grabbed).
> But, I can see the outline on hover.

OS : Windows

Comment 37

[Tim Nguyen :ntim]

This is not specific to customization mode, websites are also affected.
Gonna fill a bug.

Comment 38

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #37)
> This is not specific to customization mode, websites are also affected.
> Gonna fill a bug.

bug 977862 .

Possible solutions :
- Fix that bug
- Manually add a dragover attribute to the toolbars using JS

Comment 39

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #38)
> - Manually add a dragover attribute to the toolbars using JS

The code is meant to already do that, see the earlier comments. Matt's traveling right now. If he hasn't looked by then, I can have a look my morning tomorrow (Friday).

Comment 40

[Matthew N. [:MattN]]

Yeah, what Gijs said. I'm morphing bug 977862 to cover comment 35 but it may be related to bug 977516.

Comment 41

[:Gijs (he/him)]

Comment on attachment 8381260 [details] [diff] [review]
Part 1 - v.1.1 Provide attributes/classes to style customization targets

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: lack of visual indications about where users can drag items
Testing completed (on m-c, etc.): on m-c for quite a while
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 42

[:Gijs (he/him)]

Comment on attachment 8381350 [details] [diff] [review]
Part 2 - v.1 Add an outline on entering customization mode or upon hover/drag over customization targets

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: lack of visual indications about where users can drag items
Testing completed (on m-c, etc.): on m-c for quite a while
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 43

[:Gijs (he/him)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/1440de45f9f5
http://hg.mozilla.org/releases/mozilla-aurora/rev/0395c275b1e4