Closed Bug 1679474 Opened 4 years ago Closed 2 years ago

Remove HTMLInputElement::DispatchSelectEvent and corresponding textarea method

Categories

(Core :: DOM: Selection, defect, P3)

defect

Tracking

()

RESOLVED FIXED
106 Branch
Tracking Status
firefox106 --- fixed

People

(Reporter: saschanaz, Assigned: jjaschke)

References

(Blocks 1 open bug, )

Details

(Keywords: good-first-bug)

Attachments

(1 file)

It's used for synchronously dispatching select event, which must not occur per the spec. It's currently used in Select() (bug 1677263) and PostHandleEvent().

Severity: -- → S3
Priority: -- → P3

This is probably easy, TextControlState::SetSelectionRange should cover both DispatchSelectEvent and SelectAll.

Keywords: good-first-bug

Testcase

  1. Go to https://jsfiddle.net/d_toybox/4samc0ke/1/
  2. Press left mouse button over "a"
  3. Drag mouse to "c"
  4. Release the button

Actual Result

  • "mousedown event fired"
  • "microtask queued from mousedown event listener run"
  • "focus event fired"
  • "microtask queued from focus event listener run"
  • "timer set from mousedown event listener run"
  • "timer set from focus event listener run"
  • "select event fired"
  • "click event fired"
  • "microtask queued from click event listener run"
  • "timer set from click event listener run"

Expected Result (?)

  • "mousedown event fired"
  • "microtask queued from mousedown event listener run"
  • "focus event fired"
  • "microtask queued from focus event listener run"
  • "timer set from mousedown event listener run"
  • "timer set from focus event listener run"
  • "click event fired"
  • "microtask queued from click event listener run"
  • "select event fired"
  • "timer set from click event listener run"

This is the result on Chrome.

With a simple fix, it cannot fix my testcase in comment 2 because we set focus at post processing of mousedown event, so at this moment, mouseup has not been queued into the event loop in the content process yet. Therefore, focus event dispatched from the post processing of mousedown event causes "Select All" at focus event post processor in HTMLInputElement (where kagami pointed out). Then, select event is queued and flushed before mouseup event is coming.

Kagami, do you have any idea to test the new behavior?

(Ideally, we should check when (mousedown, mouseup or click) Chrome change Selection in <input> and align the behavior though.)

Flags: needinfo?(krosylight)

Then, select event is queued and flushed before mouseup event is coming.

Doesn't that only happen with shouldSelectAllOnFocus being true, which becomes true only by keyboard or calling .focus()? Clicking doesn't seem to have that behavior.

My log says select happens after mouseup. This is the result when I click the front of the text, drag to the right, and finish the click (with a mouseup listener added:

    "mousedown event fired"
    "microtask queued from mousedown event listener run"
    "focus event fired"
    "microtask queued from focus event listener run"
    "timer set from mousedown event listener run"
    "timer set from focus event listener run"
    "mouseup event fired"
    "microtask queued from mouseup event listener run"
    "select event fired"
    "click event fired"
    "microtask queued from click event listener run"
    "timer set from mouseup event listener run"
    "timer set from click event listener run"

Kagami, do you have any idea to test the new behavior?

Synthesize the click, get an array and push the event type on each event, wait a bit to flush all the events, and finally check the event order?

Flags: needinfo?(krosylight)

(In reply to Kagami :saschanaz from comment #4)

Then, select event is queued and flushed before mouseup event is coming.

Doesn't that only happen with shouldSelectAllOnFocus being true, which becomes true only by keyboard or calling .focus()? Clicking doesn't seem to have that behavior.

Oh? I'm confused...

My log says select happens after mouseup. This is the result when I click the front of the text, drag to the right, and finish the click (with a mouseup listener added:

    "mousedown event fired"
    "microtask queued from mousedown event listener run"
    "focus event fired"
    "microtask queued from focus event listener run"
    "timer set from mousedown event listener run"
    "timer set from focus event listener run"
    "mouseup event fired"
    "microtask queued from mouseup event listener run"
    "select event fired"
    "click event fired"
    "microtask queued from click event listener run"
    "timer set from mouseup event listener run"
    "timer set from click event listener run"

Thanks, but oddly, <input> does not select the value with clicking the <input> element in Nightly. I'll check it deeper today if I have much time to work on this.

Kagami, do you have any idea to test the new behavior?

Synthesize the click, get an array and push the event type on each event, wait a bit to flush all the events, and finally check the event order?

My point is, how to put an event into the queue between focus and select caused by the call of SelectAll. It seems that toggle event of <details> element may be available, but I'm still confused at the results.

Ah, sorry, my test case commented in 2 is completely wrong for this bug. It does not cause a call of HTMLInputElement::DispatchSelectEvent in HTMLInputElement::PostHandleEvent.

Therefore, I wrote new test case which can be tested both in Chrome and Firefox:
https://jsfiddle.net/d_toybox/tre1b4hm/2/

Then, I got the following events in Firefox:

  • focus
  • select
  • toggle
  • select

But I got the following events in Chrome:

  • focus
  • select
  • toggle

Then, I tested the patch of Jan, commenting out the DispatchSelectEvent call. Then, I got:

  • focus
  • toggle
  • select

So the event order is still different, but according to the spec of <details>, it should be dispatched synchronously.

And I check same thing with error event of <img>. However, in this case, error event and select event order is also opposite between Firefox and Chrome.

My conclusion is, we cannot check exact same behavior between browsers. Perhaps, we can just check whether there is or no redundant select event and whether setTimeout(, 0) runs before/after select event. (Chrome selects all before dispatching focus event, so we cannot check it in WPT.) Perhaps, the patch should come with a mochitest-plain or a Mozilla specific WPT.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #6)

And I check same thing with error event of <img>. However, in this case, error event and select event order is also opposite between Firefox and Chrome.

<img> with src attribute synchronously loads image currently in Gecko and error event is fired asynchronously, so we get the error event before select event is kind of expected.

(Chrome selects all before dispatching focus event, so we cannot check it in WPT.)

So it looks like Chrome schedule select event dispatching before running focus event handler, too.

Assignee: nobody → jjaschke
Status: NEW → ASSIGNED
Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/7ab1f393da19 Removed DispatchSelectEvent from HTMLInputElement::PostHandleEvent. r=masayuki
Pushed by imoraru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/630397ba4155 fix lint failure on dispatch_select_event.html. r=fix CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: