382369 - Cannot select text in <label> associated with a <select>

Status: RESOLVED FIXED

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

Description

[Adam Guthrie]

Attachment: testcase

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070529 Minefield/3.0a5pre

Steps to reproduce:
1. Load testcase.
2. Try to select text in the <label>, but don't start from the beginning.

Results:
After I release my mouse, the text is unselected and the <select> element gets a focus outline.

Expected Results:
The text remains selected.

Note that if you start selecting from the beginning of the <label> the text will remain selected. This might be the expected behavior, but I don't think it's always done this.

Comment 1

[Adam Guthrie]

This regressed between 2006-12-01-04 and 2006-12-02-04 on Linux trunk. http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-01+03&maxdate=2006-12-02+05&cvsroot=%2Fcvsroot

Pretty sure bug 360009 caused this, but I'm still unsure as to whether this is the correct/incorrect behavior.

Comment 2

[Mats Palmgren (inactive)]

I think it's a bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Nominating, since this is a regression.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The condition for whether it works correctly seems to be whether the select itself is inside the selection.

Comment 5

[Andrew Smith]

I'll give this one a shot.

Comment 6

[Andrew Smith]

Mats, is there a reason why you used eEventFocusedByKey in nsHTMLLabelElement::PostHandleEvent() (formerly HandleDOMEvent())? At a quick look it seems it should have been eEventFocusedByMouse.

Not that I think that has to do with thins bug, since nsHTMLLabelElement::PostHandleEvent() doesn't seem to be called at all.

Comment 7

[Andrew Smith]

Attachment: fix using eEventFocusedByMouse

Aha, or else I don't know how to rebuild content. I believe this is the fix.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This looks reasonable to me, but you should really ask Mats Palmgren (mats.palmgren@, I think) for review.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 280546 [details] [diff] [review]
fix using eEventFocusedByMouse

Using eEventFocusedByKey was intentional.  It affects scrolling to
the target (for-content) and the select-all-on-focus behaviour
of the target (see Testcase #2).

We also want to completely avoid clicks that are associated
with a selection gesture (e.g. selecting text in the label of a
checkbox should not toggle the check box).

Comment 11

[Mats Palmgren (inactive)]

Attachment: Alternative patch, rev. 1

This patch checks for a mouse grabber during MOUSE_MOVE and sets a bit
that will cause the next click to be ignored. (Let me know if there's
a better heuristic for detecting a drag-selection.)
I also excluded clicks with a kbd modifier since those are usually
used for adjusting the selection (or table cell selection).
I also ignored the second click of a double-click, the first event
will be passed through, but ignoring the second event allows for
word-selection on the label.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about just checking whether the mouse-up part of the click is too far away from the mouse-down part of the click and suppressing the click in that case?

Comment 13

[Mats Palmgren (inactive)]

Attachment: Alternative patch, rev. 2

Measure the distance from the mouse-down -- assume it's a selection gesture
if more than 2 pixels.  It's actually possible to select a letter in less
than that (if you click near the middle of a letter and drag just over the
mid-point to the other half of the letter), but I think this is good enough.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  struct MouseDownData {
+    nsPoint      mRefPoint;
+  };
+  MouseDownData* mMouseDownData;

I think this is a perfect place to use nsINode::Get/SetProperty. And I wouldn't bother with the MouseDownData wrapper.

Should we store coordinates relative to this element or document or something? There's no guarantee that the widget in the click event is the same as the widget in the down event ... is there?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We really should fix this, the fix is in reach. But we wouldn't block on it.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mats, can you follow up here? This seems close

Comment 20

[John Daggett (:jtd)]

This affects a lot of sites.  Mats, any chance we can get the patch updated and checked in?

Comment 22

[Gen Kanai [:gen]]

I'd like to add my request that this be fixed asap, as it is a regression and that it has been an issue for almost 2 years.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Zack, would you mind finishing this one off?

Comment 24

[Zack Weinberg (:zwol)]

Attachment: updated patch, no memory allocation

Here's a patch that applies to the current tree.  Re roc's suggestions in comment 14:

 - I made the nsIntPoint a direct member of nsHTMLLabelElement; that avoids all memory allocation.  nsINode::*Property seem like massive overkill for this.

 - There doesn't appear to be any straightforward way to get anything but widget coordinates at this point in the code.  We don't have a DOM event.  We could grovel through the frame tree, or possibly the view tree, and get the bounding rect of the element relative to the document, but it appears to be a whole lot of work (look at the hoops nsPresShell::ScrollContentIntoView goes through, for instance) and given that we are talking about one user mouse operation, I don't see any reason to think it _wouldn't_ be the same widget, particularly since we are moving toward one-widget-per-toplevel-window...

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ignoring the widget issue is OK I guess, but I really would like to keep the point in a property. 8 bytes per label element is small but seems worth avoiding for something that's basically never used, and avoiding it is easy.

Comment 26

[Zack Weinberg (:zwol)]

Attachment: revised, now uses properties

*shrug* here's a version using Get/SetProperty.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+UpdateMouseDownPoint(nsHTMLLabelElement *aNode,
+                            nsIntPoint &aRefPoint)

Fix indent

+  if (curPoint)
+    *curPoint = aRefPoint;
+  else {

Statement needs {}

UpdateMouseDownPoint could be simplified to just always call SetProperty --- no perf issues here, not worth avoiding the extra allocation. In fact you could just get rid of the function and inline the SetProperty call.

+    = (nsIntPoint*)aNode->GetProperty(nsGkAtoms::labelMouseDownPtProperty);

static_cast

+    aNode->SetProperty(nsGkAtoms::labelMouseDownPtProperty, (void *)curPoint,

static_cast

+          nsIntPoint *mouseDownPoint
+            = (nsIntPoint *)GetProperty(nsGkAtoms::labelMouseDownPtProperty);

static_cast

Comment 28

[Zack Weinberg (:zwol)]

Attachment: v3, unconditional SetProperty

revised as requested

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/934ae18dd58f

Thanks for helping out, Zack.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 368954 [details] [diff] [review]
v3, unconditional SetProperty

This is a regression that has been bothering people for a while, we should probably take it on 1.9.1. Risk is low.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we could probably have an automated test for this using synthetic mouse events.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 368954 [details] [diff] [review]
v3, unconditional SetProperty

a1.9.1=dbaron

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e9a8f3b9ff0a