Last Comment Bug 722100 - Use ondblclick instead of hard-coded time interval to detect double clicks on ui.js
: Use ondblclick instead of hard-coded time interval to detect double clicks on...
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 13
Assigned To: Reuben Morais [:reuben]
:
:
Mentors:
https://hg.mozilla.org/mozilla-centra...
Depends on: 722144 728887
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-28 15:56 PST by Reuben Morais [:reuben]
Modified: 2016-04-12 14:00 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (4.70 KB, patch)
2012-01-28 20:05 PST, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Patch with drag-to-create-group and fixed tests (6.80 KB, patch)
2012-01-29 04:57 PST, Reuben Morais [:reuben]
no flags Details | Diff | Splinter Review
Patch v3 (8.72 KB, patch)
2012-02-02 09:51 PST, Reuben Morais [:reuben]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch v4 (8.08 KB, patch)
2012-02-03 03:51 PST, Reuben Morais [:reuben]
ttaubert: review+
Details | Diff | Splinter Review

Description Reuben Morais [:reuben] 2012-01-28 15:56:01 PST
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] 2012-01-28 15:59:28 PST
(Same applies to DBLCLICK_OFFSET right under that, of course)
Comment 2 Reuben Morais [:reuben] 2012-01-28 20:05:48 PST
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.
Comment 3 Tim Taubert [:ttaubert] 2012-01-28 22:04:41 PST
(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] 2012-01-28 22:05:13 PST
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] 2012-01-29 04:57:50 PST
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.
Comment 6 Tim Taubert [:ttaubert] 2012-01-30 07:48:06 PST
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] 2012-01-31 04:06:48 PST
(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] 2012-02-01 06:32:27 PST
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] 2012-02-02 09:51:59 PST
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
Comment 10 Tim Taubert [:ttaubert] 2012-02-03 03:04:38 PST
(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] 2012-02-03 03:36:33 PST
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] 2012-02-03 03:51:02 PST
Created attachment 594117 [details] [diff] [review]
Patch v4
Comment 13 Tim Taubert [:ttaubert] 2012-02-03 03:52:52 PST
Comment on attachment 594117 [details] [diff] [review]
Patch v4

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

Thank you!
Comment 14 Tim Taubert [:ttaubert] 2012-02-03 05:14:03 PST
https://hg.mozilla.org/integration/fx-team/rev/16992ae5a38a

Thank you, Reuben!
Comment 15 Tim Taubert [:ttaubert] 2012-02-04 02:40:01 PST
https://hg.mozilla.org/mozilla-central/rev/16992ae5a38a

Note You need to log in before you can comment on or make changes to this bug.