722100 - Use ondblclick instead of hard-coded time interval to detect double clicks on ui.js

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Reuben Morais [:reuben]]

https://hg.mozilla.org/mozilla-central/file/79d0b6168a53/browser/components/tabview/ui.js#l54

We should probably use ondblclick there, a hard-coded interval not a11y friendly.

Comment 1

[Reuben Morais [:reuben]]

(Same applies to DBLCLICK_OFFSET right under that, of course)

Comment 2

[Reuben Morais [:reuben]]

Attachment: Patch

While I was at it…
Really nice code in tabview, btw. Modularized and "webby", could be a nice place for new contributors who are familiar with HTML/JS/CSS for the Web but not for the browser.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Reuben Morais [:reuben] from comment #2)
> Really nice code in tabview, btw. Modularized and "webby", could be a nice
> place for new contributors who are familiar with HTML/JS/CSS for the Web but
> not for the browser.

Indeed, that's how I started with Firefox development!

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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

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

Thanks for your patch, Reuben!

This looks like the right way to go but there is a number of tests in browser/components/tabview/test/ that broke. Did you try running tabview tests? Just in case you've never done that before, there is some guidance here:

https://developer.mozilla.org/en/Browser_chrome_tests

::: browser/components/tabview/ui.js
@@ -239,5 @@
> -              gTabView.firstUseExperienced = true;
> -            } else {
> -              self._lastClick = Date.now();
> -              self._lastClickPositions = new Point(e.clientX, e.clientY);
> -              self._createGroupItemOnDrag(e);

Without this we remove the ability to create group items by dragging :(

Comment 5

[Reuben Morais [:reuben]]

Attachment: Patch with drag-to-create-group and fixed tests

My mistake, I built the changes on the debug objdir and ran the tests on the optimized one. This version doesn't bork the drag-to-create-group feature and fixes some broken tests. It depends on the patch in bug 722144.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 592492 [details] [diff] [review]
Patch with drag-to-create-group and fixed tests

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

Please remove the UI.DBLCLICK_INTERVAL references in browser_tabview_snapping.js as well.

Alas, there's one problem left: you removed the offset check so we now have the problem that one can create two groups by double clicking while moving the mouse fast over the screen. So that createGroupItemOnDrag() has the minimum group size and dblclick got fired. We could possibly solve that by preventing the event propagation from "mouseup" in createGroupItemOnDrag() if this has successfully created a group (didn't try that). Or we could set a flag if a group has been created by dragging that blocks the dblclick handler from doing that...

Comment 7

[Reuben Morais [:reuben]]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Alas, there's one problem left: you removed the offset check so we now have
> the problem that one can create two groups by double clicking while moving
> the mouse fast over the screen. So that createGroupItemOnDrag() has the
> minimum group size and dblclick got fired. We could possibly solve that by
> preventing the event propagation from "mouseup" in createGroupItemOnDrag()
> if this has successfully created a group (didn't try that). Or we could set
> a flag if a group has been created by dragging that blocks the dblclick
> handler from doing that...

I'm a bit lost here, as I couldn't reproduce this problem. The OS already does offset validation to trigger the double click, if I move my mouse fast over the screen and click twice it wont be a double click but two click-and-drag's.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

What OS are you using? I can reproduce it on OS X by moving the mouse fast enough after the second mousedown before releasing the mouse button.

Comment 9

[Reuben Morais [:reuben]]

Attachment: Patch v3

Okay, checking if the click count is higher than 1 appears to work. There's a slight change in behavior, though: dblclick is only fired after mouseup [1], so the tab group will only be created after the mouse is released when double clicking and dragging. The current implementation creates the group in the second mousedown, which isn't a real double click, so this behavior change is OK IMO.

[1] http://www.w3.org/TR/DOM-Level-3-Events/#events-mouseevent-event-order

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Reuben Morais [:reuben] from comment #9)
> Okay, checking if the click count is higher than 1 appears to work. There's
> a slight change in behavior, though: dblclick is only fired after mouseup
> [1], so the tab group will only be created after the mouse is released when
> double clicking and dragging. The current implementation creates the group
> in the second mousedown, which isn't a real double click, so this behavior
> change is OK IMO.

Yes, I don't think we intentionally picked 'mousedown' in the old implementation, it was just easier to pair with creating a group item on drag.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 593890 [details] [diff] [review]
Patch v3

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

Thanks, that looks good and works well! There is only a minor modification left.

::: browser/components/tabview/ui.js
@@ +197,5 @@
>                element.blur();
>            });
>          }
> +        if (e.originalTarget.id == "content" &&
> +            Utils.isLeftClick(e)) {

Please add 'e.detail == 1' to the condition here and remove the modifications in _createGroupItemOnDrag().

@@ +1355,5 @@
> +    // Don't create two groups after a double-click-and-drag
> +    if (e.detail == 1) {
> +      iQ(window).mousemove(updateSize)
> +      iQ(gWindow).one("mouseup", finalize);
> +    }

Please remove this in favor of the modified condition above.

Comment 12

[Reuben Morais [:reuben]]

Attachment: Patch v4

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 594117 [details] [diff] [review]
Patch v4

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

Thank you!

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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

Thank you, Reuben!

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/16992ae5a38a