968565 - URL bar and search container resizer gets removed because of hidden items

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Filing on behalf of a tweet I saw[1] about this, and hopefully the user can fill in some more details, specifically:

1) When this started
2) What operating system they're using
3) Minimal steps to reproduce

I'm not adding a priority yet until we have more information.

[1]: https://twitter.com/croncobaurul/status/431212812212248577

Comment 1

[:Gijs (he/him)]

I think what tends to be the problem when I've looked at this is that there are hidden items (like the webrtc status button) inbetween them, after you drag bits and pieces around. To debug this, the output of the following JS in the browser console would be very useful:

CustomizableUI.getWidgetIdsInArea("nav-bar").join(',');

Comment 2

[Mihai Chereji]

Hi, I'm the original tweeter.

I'm currently running Ubuntu Trusty Tahr developer previews, with Nightly v30. from 05.02.2014 from the ubuntu-mozilla-daily PPA.

1. I didn't experience the behaviour with previous nightlies, AFAIK (I can reinstall & check.)
3. Steps: 
 - Start Nigthly. 
 - Try resizing URL/search bar.
 - Handler does not appear.
 - Try Same actions in Customize mode
 - Handler still fails to appear.

I'll try repeating the steps with a clean profile.

Comment 3

[Mihai Chereji]

Ok, just tried with a clean profile, and the problem was not encountered.

Here's the output from CustomizableUI.getWidgetIdsInArea("nav-bar").join(',');

"webrtc-status-button,urlbar-container,social-share-button,social-toolbar-item,search-container,pbff-pinmenu,downloads-button,email-link-button,bookmarks-menu-button,RIL_toolbar_button,sessionmanager-toolbar,nav-bar-customization-target,nav-bar-overflow-button,PanelUI-button,window-controls,twitbin-button,clickclean-button,firebug-button"

Comment 4

[Mihai Chereji]

@Gijs is right: Having any widget in between the two bars disables resizing them.

And for some reason I can't delete (Using CustomizableUI.removeWidgetFromArea) social-share-button from the nav-bar area, which is between the two bars, and so I can't resize them.

Comment 5

[:Gijs (he/him)]

(In reply to Mihai Chereji from comment #4)
> @Gijs is right: Having any widget in between the two bars disables resizing
> them.
> 
> And for some reason I can't delete (Using
> CustomizableUI.removeWidgetFromArea) social-share-button from the nav-bar
> area, which is between the two bars, and so I can't resize them.

This is because you can't remove those widgets from the navbar, they're marked as non-removable (we're working to fix that, but it's been like that for many releases). You can move them to the end, though (which is where they are by default) or you can move the search box to be directly next to the navbar. If you drag the search box onto the navbar so that it drops in to its right, that should fix things (although we never show the splitter inside customization mode, so you'll need to go out of customization mode to test it).

Alternatively:

CustomizableUI.moveWidgetWithinArea("social-share-button", 10000);

will move the hidden button to the end of the area.

(any large number will do, I'm too lazy to check what the correct value is - we normalize these numbers anyway)


Now, as to what we can do about this... that's more difficult.

We can't ignore hidden elements when it comes to deciding whether we show the splitter, because then if you unhide those elements we won't know (and adding mutation observers on all of them seems very icky).

We could make the drag and drop code always put items you move before hidden items, but that has problems if you're trying to control where things like the webrtc status button / social share buttons end up when they're not hidden (and the webrtc status button will always be hidden when you use customization mode, as that page obviously doesn't use webrtc...)

Not sure what to do here. Mike or Mihai, do you have ideas? :-)

Comment 6

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

(In reply to :Gijs Kruitbosch from comment #5)
> We could make the drag and drop code always put items you move before hidden
> items, but that has problems if you're trying to control where things like
> the webrtc status button / social share buttons end up when they're not
> hidden (and the webrtc status button will always be hidden when you use
> customization mode, as that page obviously doesn't use webrtc...)
> 

This might be the best option here. My gut tells me that users attempting to manipulate where hidden items are in the toolbar is an edge-case, but users ending up with hidden things screwing up the interactions between items is more likely.

So I think I'd be in for it.

For users that really do want to control where the hidden items are in the toolbar, a userchrome or add-on could force them to be displayed. I'm willing to bet users who want to control the position of the hidden items are likely technical, and could adopt a solution like that with little difficulty.

Comment 7

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

Attachment: WIP patch

This is pretty half-baked, but I started on this. jaws wants to grab it, so here's what I've got.

Comment 8

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

Attachment: Patch

Comment 9

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

Comment on attachment 8386310 [details] [diff] [review]
Patch

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +1282,5 @@
>              dropTargetCenter += (parseInt(dragOverItem.style.borderLeftWidth) || 0) / 2;
>            } else {
>              dropTargetCenter -= (parseInt(dragOverItem.style.borderRightWidth) || 0) / 2;
>            }
> +          let before = direction == "ltr" ? aEvent.clientX < dropTargetCenter : aEvIent.clientX > dropTargetCenter;

typo -> aEvIent -> aEvent.

@@ +1879,5 @@
>        target.setAttribute("customizing-dragovertarget", true);
>      }
>    },
>  
> +  _findVisibleSiblingNode: function(aReferenceNode) {

To be clearer, let's call this _findVisiblePreviousSiblingNode.

::: browser/components/customizableui/test/browser_968565_insert_before_hidden_items.js
@@ +9,5 @@
> +
> +let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
> +
> +// When we drag an item onto a customizable area, and not over a specific target, we
> +// should assume that we're appending them to the area. If doing so, we should scan

Can we also add the test case where we drag not onto the customizationTarget, but before an item that has hidden items before it? (which may have been injected by add-ons or something).

Comment 10

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

Attachment: Patch v2

Can you please run these tests on your machine? The HiDPI on my machine is causing issues with drag+drop.

Comment 11

[:Gijs (he/him)]

Attachment: [Australis] When inserting a customizable item before another item or at the end of an area, skip over hidden items. r=?

Comment 12

[:Gijs (he/him)]

Comment on attachment 8386460 [details] [diff] [review]
[Australis] When inserting a customizable item before another item or at the end of an area, skip over hidden items. r=?

r=me with this test fix and the debugger; statement removed.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8386460 [details] [diff] [review]
[Australis] When inserting a customizable item before another item or at the end of an area, skip over hidden items. r=?

... but please doublecheck my test fix.

Comment 14

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

Comment on attachment 8386460 [details] [diff] [review]
[Australis] When inserting a customizable item before another item or at the end of an area, skip over hidden items. r=?

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

Thanks!

::: browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js
@@ +78,5 @@
> +  }
> +  is(getLastVisibleNodeInToolbar(containerId).firstChild.id, id,
> +     "Widget " + id + " should be in " + containerId + " in customizing window.");
> +  is(getLastVisibleNodeInToolbar(containerId, otherWin).id, id,
> +     "Widget " + id + " should be in " + containerId + " in other window.");

These aren't just checking that the widget is in the window, but also that it is the last visible item, so these two assertion comments should be updated.

Comment 15

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

https://hg.mozilla.org/integration/fx-team/rev/566318203819

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/566318203819

Comment 17

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

Attachment: Patch for Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): issue since the beginning of Australis
User impact if declined: the urlbar-search container splitter will not get inserted if hidden items end up between them.
Testing completed (on m-c, etc.): locally, just landed on m-c, and there is an automated test with it.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/262af03ee2c6

Comment 19

[Ioana (away)]

I see this bug is both "verifyme" and "qa-". Given it's also in-testsuite+ (covered automatically), I'll remove the "verifyme".

Tracy, if you consider there is something here that needs manual coverage besides the automated one, please remove "qa-", re-add "verifyme" and give more details about it. Thanks

Comment 20

[Tracy Walker [:tracy]]

heh, the verifyme was meant for another bug.  perils of working in multiple tabs on multiple bugs at once.  Thanks Ioana.