Closed Bug 968565 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: mconley, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3])

Attachments

(2 files, 3 obsolete files)

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
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(',');
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.
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"
@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.
(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? :-)
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Summary: URL bar and search container resizer is not working → URL bar and search container resizer is not due to hidden items
Whiteboard: [Australis:P3]
Summary: URL bar and search container resizer is not due to hidden items → URL bar and search container resizer gets removed because of hidden items
Assignee: nobody → mconley
Attached patch WIP patch (obsolete) — Splinter Review
This is pretty half-baked, but I started on this. jaws wants to grab it, so here's what I've got.
Assignee: mconley → jaws
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Attachment #8384866 - Attachment is obsolete: true
Attachment #8386310 - Flags: review?(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).
Attachment #8386310 - Flags: review?(mconley)
Attached patch Patch v2 (obsolete) — Splinter Review
Can you please run these tests on your machine? The HiDPI on my machine is causing issues with drag+drop.
Attachment #8386310 - Attachment is obsolete: true
Attachment #8386416 - Flags: review?(gijskruitbosch+bugs)
Attachment #8386416 - Attachment is obsolete: true
Attachment #8386416 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8386460 - Flags: review+
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.
Attachment #8386460 - Flags: review?(jaws)
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.
Attachment #8386460 - Flags: review?(jaws) → review+
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/566318203819
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Attached patch Patch for AuroraSplinter Review
[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
Attachment #8387336 - Flags: approval-mozilla-aurora?
Attachment #8387336 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [qa-]
Keywords: verifyme
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
Keywords: verifyme
heh, the verifyme was meant for another bug.  perils of working in multiple tabs on multiple bugs at once.  Thanks Ioana.
See Also: → 1172651
You need to log in before you can comment on or make changes to this bug.