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

RESOLVED FIXED in Firefox 13

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

Trunk
Firefox 13
Dependency tree / graph

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
(Same applies to DBLCLICK_OFFSET right under that, of course)
(Assignee)

Comment 2

6 years ago
Created attachment 592457 [details] [diff] [review]
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.
Assignee: nobody → reuben.morais
Attachment #592457 - Flags: review?(ttaubert)
(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!
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
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 :(
Attachment #592457 - Flags: review?(ttaubert)
(Assignee)

Updated

6 years ago
Depends on: 722144
(Assignee)

Comment 5

6 years ago
Created attachment 592492 [details] [diff] [review]
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.
Attachment #592457 - Attachment is obsolete: true
Attachment #592492 - Flags: review?(ttaubert)
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...
Attachment #592492 - Flags: review?(ttaubert)
(Assignee)

Comment 7

6 years ago
(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.
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.
(Assignee)

Comment 9

6 years ago
Created attachment 593890 [details] [diff] [review]
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
Attachment #592492 - Attachment is obsolete: true
Attachment #593890 - Flags: review?(ttaubert)
(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 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.
Attachment #593890 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 12

6 years ago
Created attachment 594117 [details] [diff] [review]
Patch v4
Attachment #593890 - Attachment is obsolete: true
Attachment #594117 - Flags: review?(ttaubert)
Comment on attachment 594117 [details] [diff] [review]
Patch v4

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

Thank you!
Attachment #594117 - Flags: review?(ttaubert) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/16992ae5a38a

Thank you, Reuben!
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/16992ae5a38a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Depends on: 728887
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.