1276126 - Label with user-select:none acts like a normal text when it comes to preventing events on checkbox

Status: NEW

(Core :: Layout: Form Controls, defect, P4)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR:
1. Open the following "data:" url or click URL in the form above
> data:text/html,<input><label for="first"><input id="first" type="checkbox">First</label><label for="second"><input id="second" type="checkbox">Second</label><label for="third"><input id="third" type="checkbox">third</label><label>abcd</label><style>label{-moz-user-select:-moz-none;-webkit-user-select:none;} *>:focus{outline:1px dashed red}
2. Move mouse between "o" and "n" on the page, hold left mouse button,
   move mouse between "e" and "c" on the page, release left mouse button
3. Doubleclick on the letter "e"

AR:  After Step 2 nothing happens on the page. After Step 3 checkbox isn't focused
ER:  After Step 2 checkbox should be checked. After Step 3 checkbox should be still focused

It caused regression in devtools options as they switched from XUL checkboxes to HTML in bug 1246514.
NI for Olli: please rate this behavior in 2 catgories - (a) good/bad  (b) follows the spec or not.
1) "click and drag" doesn't work on <label>
2) checkbox looses focus after doubleclick on <label>

This is regression from bug 382369. Regression range (between 2009-03-23 and 2009-03-24):
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d74e551f46a3&tochange=6cfe70091cfc

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

mats might have an opinion on this one.

I don't know why after 2 checkbox should be checked.
checkbox not being focused after 3 is a bit weird, so that at least should be consistent with clicking just once.

Comment 2

[arni2033]

(In reply to Olli Pettay [:smaug] from comment #1)
> I don't know why after 2 checkbox should be checked.
There're several reasons (some may be questionable). Here's full explanation why I have reported this:

1) Spec(?)
I most likely read it wrong, but [1] -> [2] -> [3] seems the right chain for comment 0.
Actually [3] isn't necessary, because 'click' event fires on <label> - that's for sure. Therefore
checkbox should be clicked. The only excuse not to click checkbox is the presence of text selection
- that's per your comment (bug 1037941 comment 2). But there's no text selection in STR in comment 0.
[1] https://html.spec.whatwg.org/#the-label-element
[2] https://html.spec.whatwg.org/#activation
[3] https://w3c.github.io/uievents/#event-type-click

2) Real intention of bug 382369
Open http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLLabelElement.cpp#158
and scroll that line to the middle of the window (to read text below and above).
Text above checks for selection by looking at mouse coordinate(sic), but the most important part is:
> 156           // Don't click the for-content if we did drag-select text or if we
> 157           // have a kbd modifier (which adjusts a selection).
As you may see, the person who made that patch says "drag-select" and "selection", so he didn't really expect that we're not selecting text at the moment.
The patch is called "Distinguish click from click-drag on a <label> so text selection works".
Therefore original intent of bug382369 was to fix text selection. Not to touch "user-select:none" case

3) GoogleChrome works this way
Chrome works as expected on STR in comment 0. If I remove "user-select:none", it behaves like Firefox,
i.e. allows to select text without clicking checkbox. So web devs will mostly expect this behavior.

4) UX
It's too easy to accidentally move mouse pointer by ~2px while clicking, (especially) on touchpad or if I'm looking at something else on the page, thinking whether I'm doing the right thing or not. When <checkbox>es in devtools options switched to HTML, I noticed it right away, as clicking became harder.


> checkbox not being focused after 3 is a bit weird, so that at least should
> be consistent with clicking just once.
Please use this bug for any sensible valid bug mentioned in comment 0 (if there're no duplicates).

Comment 3

[Mats Palmgren (inactive)]

It appears we fire a click event on the <label>, but not on the associated
checkbox.  I think we should probably not fire a click event at all since
the user is actually doing a drag gesture.  We don't fire click when
dragging on an <input type=button> and I think a "user-select:none" label
should probably work the same.  I would be surprised if any spec says
that we should fire a click for a drag gesture.

> It's too easy to accidentally move mouse pointer by ~2px while clicking

So isn't this a problem for ordinary buttons too then?

If the threshold distance to initiate a drag operation is too small on
some platforms then we should probably increase it on those platforms
to fix the problem more generally.

> checkbox looses focus after doubleclick on <label>

That seems like a (minor) bug.  It might be hard to fix and it's a very
rare edge case, so probably not worth spending our limited resources on.

Comment 4

[arni2033]

Attachment: bug 1276126 comment 4.webm

(In reply to Mats Palmgren (:mats) from comment #3)
> We don't fire click when dragging on an <input type=button>
That's probably a typo. Is it? You do fire click when dragging on an <input type=button> , see url [1]

> <input type=button> and I think a "user-select:none" label should probably work the same
Nice! Then make "user-select:none" label work the same as <input type=button>

> I would be surprised if any spec says that we should fire a click for a drag gesture.
It works this way on every element. This is either a big typo, or a huge mistake in implementing behavior described above (not firing click for "drag gesture")

> That seems like a (minor) bug. It might be hard to fix and it's a very
> rare edge case, so probably not worth spending our limited resources on.
And as always, bug 382369 is itself a minor bug, but you still chose to "fix" it breaking usability (see 3 open dependent bugs).

[1]   data:text/html,<input type="button" value="asdf" onclick="console.log('click')" onmousedown="console.log('down')" onmouseup="console.log('up')"></input>

Comment 5

[Mats Palmgren (inactive)]

> That's probably a typo. Is it?

You're right, I must have made a mistake when I tested the button behavior.

So I agree that step 2 in the STR should generate a normal click event
so that a "user-select:none" <label> behaves the same as a button for
that gesture.

Comment 6

[arni2033]

(In reply to Mats Palmgren (:mats) from comment #5)
> So I agree that step 2 in the STR should generate a normal click event
Well, that is not the bug in Step 2... Browser already generates click event on <label>
The bug is (in my view), that click event isn't passed to inner <input>

If I'm reading https://html.spec.whatwg.org/#the-label-element and then https://html.spec.whatwg.org/#activation correctly, click event should be passed to <input>:   I just tested a native Windows checkbox in Mozregression GUI ("platform behavior"), and it matches the expectations for Step 2.
It was decided in bug 382369 that text selection should be an excuse for not following the spec,
that's kind of OK. But I assume that in Step 2 there's no excuse, since there's no text selection.

It doesn't matter how much resources you have currently. The 2 things that matter are:
(1) clear understanding of what is right and (2) do you want to fix it ever (e.g. in 10 years) or not

BTW, bug in Step 3 happens for the same reason: logic in bug 382369 only expects text selection on doubleclick, therefore doesn't pass the second click to <input> :

From attachment 368954 [details] [diff] [review] (but the logic still only expects text selection)
270          // Don't click the for-content if we did drag-select text or if we
271          // have a kbd modifier (which adjusts a selection), or if it's a
272          // double click (we already forwarded the first click event).
273          if (dragSelect || event->clickCount > 1 ||
274              event->isShift || event->isControl || event->isAlt ||
275              event->isMeta) {
276            break;
277          }

Comment 7

[Mats Palmgren (inactive)]

> The bug is (in my view), that click event isn't passed to inner <input>

That's what I meant when I said "generate a normal click event" -  the click
event should be forwarded to its target.

Comment 8

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)