Fixed issue #11651 (Do not fire a blur event when calling .focus() on… #11665

davideGiovannini · 2016-06-07T18:50:25Z

Added check in HTMLElement.Focus to avoid requesting focus when the element already has it.
Added check in Document.commit_focus_transaction to avoid sending blur and focus events when clicking on an already focused element.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #11651

There are tests for these changes OR
These changes do not require tests because _____

This change is


        Fixed issue #11651 (Do not fire a blur event when calling .focus() on…

… a focused element)

highfive · 2016-06-07T18:50:31Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

highfive · 2016-06-07T18:50:32Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/document.rs, components/script/dom/htmlelement.rs

highfive · 2016-06-07T18:50:33Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-06-07T19:07:24Z

This PR could use a test.

davideGiovannini · 2016-06-07T19:28:46Z

I would like to write one, but I need some hints.

KiChjang · 2016-06-07T20:20:45Z

@paulrouget already has a test case listed in #11651. You'll need to use ./mach create-wpt path/to/test/file to create tests. Refer to https://github.com/servo/servo/blob/master/tests/wpt/README.md for more information.

paulrouget · 2016-06-09T04:57:03Z

@davideGiovannini let me know if you need help to write the test.

jdm · 2016-06-09T12:46:20Z

I haven't started reviewing the changes yet, just making note of my own investigations:
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4259 shows

log: focusing
log: focus
log: focused
log: double focused

in FF, Safari, and Chrome.

paulrouget · 2016-06-09T12:59:47Z

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4259 shows: […]

With this PR, we get the same output.

I used this:

<!DOCTYPE html>
<input>
<script>
  var w = a => console.log(a);
  var e = document.querySelector('input');
  e.addEventListener("focus", function() { w('focus') });
  e.addEventListener("blur", function() { w('blur') });
  w('focusing');
  e.focus();
  w('focused');
  e.focus();
  w('double focused');
</script>

jdm · 2016-06-09T13:26:36Z

-S-awaiting-review +S-needs-code-changes

Previously, paulrouget (Paul Rouget) wrote…

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4259 shows: […]

With this PR, we get the same output.

I used this:

<!DOCTYPE html>

<input>

<script>

  var w = a => console.log(a);

  var e = document.querySelector('input');

  e.addEventListener("focus", function() { w('focus') });

  e.addEventListener("blur", function() { w('blur') });

  w('focusing');

  e.focus();

  w('focused');

  e.focus();

  w('double focused');

</script>

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/document.rs, line 601 [r1] (raw file):

    /// transaction, or none if no elements requested it.
    pub fn commit_focus_transaction(&self, focus_type: FocusType) {
        if self.focused != self.possibly_focused.get().r() {

Let's invert this conditional and return, to avoid the extra indentation for everything else in the method.

components/script/dom/htmlelement.rs, line 254 [r1] (raw file):

        // TODO: Mark the element as locked for focus and run the focusing steps.
        // https://html.spec.whatwg.org/multipage/#focusing-steps
        if self.upcast::<Element>().focus_state() {

I don't think this change is still necessary?

Comments from Reviewable

davideGiovannini · 2016-06-09T14:19:18Z

@paulrouget Yes please, I'm not familiar with web platform tests and I'm not finding the documentation too helpful

paulrouget · 2016-06-09T15:29:02Z

@davideGiovannini maybe this will help: https://github.com/servo/servo/blob/master/docs/HACKING_QUICKSTART.md#add-a-new-test

Maybe you should create a test called tests/wpt/mozilla/tests/mozilla/double_focus.html. You might want to look in this directory to see how tests are written. You don't need an async test for this I believe.

highfive · 2016-06-10T09:34:28Z

New code was committed to pull request.


        fixup! Fixed issue #11651 (Do not fire a blur event when calling .foc…

…us() on a focused element)

KiChjang · 2016-06-13T08:58:49Z

@davideGiovannini Any progress on adding tests?

nox · 2016-06-20T11:06:09Z

@bors-servo try

bors-servo · 2016-06-20T11:06:12Z

⌛ Trying commit 01c3640 with merge 94178db...


        Auto merge of #11665 - davideGiovannini:master, r=<try>

Fixed issue #11651 (Do not fire a blur event when calling .focus() on…  Added check in `HTMLElement.Focus` to avoid requesting focus when the element already has it. Added check in `Document.commit_focus_transaction` to avoid sending `blur` and `focus` events when clicking on an already focused element. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11651  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11665)

bors-servo · 2016-06-20T12:36:26Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

paulrouget · 2016-06-20T16:52:00Z

If this lands, I can write the tests in a follow-up issue right away.

davideGiovannini · 2016-06-24T11:23:46Z

Sorry for the delay.
I tried to write the test, but I encountered some problems and unfortunately I don't think I can resume working on it untill the end of this exam session (mid July).

KiChjang · 2016-06-24T14:02:37Z

I'm going to land this and track the progress of writing a test in #11854.

@bors-servo r+

bors-servo · 2016-06-24T14:02:38Z

📌 Commit 01c3640 has been approved by KiChjang

bors-servo · 2016-06-24T14:02:40Z

⌛ Testing commit 01c3640 with merge 908e504...


        Auto merge of #11665 - davideGiovannini:master, r=KiChjang

Fixed issue #11651 (Do not fire a blur event when calling .focus() on…  Added check in `HTMLElement.Focus` to avoid requesting focus when the element already has it. Added check in `Document.commit_focus_transaction` to avoid sending `blur` and `focus` events when clicking on an already focused element. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11651  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11665)

bors-servo · 2016-06-24T14:46:04Z

💔 Test failed - linux-rel

KiChjang · 2016-06-24T14:53:15Z

@bors-servo retry

#InfraOnFire

bors-servo · 2016-06-24T15:25:03Z

⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only android, linux-rel...

bors-servo · 2016-06-24T16:16:14Z

💔 Test failed - linux-rel

KiChjang · 2016-06-24T16:26:13Z

@bors-servo retry

#11703

bors-servo · 2016-06-24T16:45:24Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

bors-servo · 2016-06-24T17:37:12Z

💔 Test failed - linux-rel

cbrewster · 2016-06-25T04:34:34Z

@bors-servo retry

#11693

bors-servo · 2016-06-25T04:34:38Z

⌛ Testing commit 01c3640 with merge 7d978e7...


        Auto merge of #11665 - davideGiovannini:master, r=KiChjang

Fixed issue #11651 (Do not fire a blur event when calling .focus() on…  Added check in `HTMLElement.Focus` to avoid requesting focus when the element already has it. Added check in `Document.commit_focus_transaction` to avoid sending `blur` and `focus` events when clicking on an already focused element. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11651  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11665)

bors-servo · 2016-06-25T05:26:35Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows


        Test for double focus events (covers servo#11665)


        Auto merge of #11865 - paulrouget:doubleFocusTest, r=ConnorGBrewster

Test for double focus events (covers #11665)  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11854 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11865)


        Auto merge of #11865 - paulrouget:doubleFocusTest, r=ConnorGBrewster

Test for double focus events (covers #11665)  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #11854 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11865)

Fixed issue #11651 (Do not fire a blur event when calling .focus() on…

Loading status checks…

35aa24e

… a focused element)

highfive assigned jdm Jun 7, 2016

highfive added the S-awaiting-review label Jun 7, 2016

KiChjang added the S-needs-tests label Jun 7, 2016

jdm added the S-needs-code-changes label Jun 9, 2016

jdm removed the S-awaiting-review label Jun 9, 2016

highfive removed the S-needs-code-changes label Jun 10, 2016

highfive added the S-awaiting-review label Jun 10, 2016

fixup! Fixed issue #11651 (Do not fire a blur event when calling .foc…

Loading status checks…

01c3640

…us() on a focused element)

jdm removed the S-awaiting-review label Jun 10, 2016

highfive assigned KiChjang and unassigned jdm Jun 24, 2016

highfive added the S-awaiting-merge label Jun 24, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 24, 2016

highfive added the S-tests-failed label Jun 24, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 25, 2016

bors-servo merged commit 01c3640 into servo:master Jun 25, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 25, 2016

paulrouget added a commit to paulrouget/servo that referenced this pull request Jun 25, 2016

Test for double focus events (covers servo#11665)

2c35233

paulrouget mentioned this pull request Jun 25, 2016

Test for double focus events (covers #11665) #11865

Merged

4 of 5 tasks complete

servo / servo

Fixed issue #11651 (Do not fire a blur event when calling .focus() on… #11665

Fixed issue #11651 (Do not fire a blur event when calling .focus() on… #11665

davideGiovannini commented Jun 7, 2016 •

edited by larsbergstrom

highfive commented Jun 7, 2016

highfive commented Jun 7, 2016

highfive commented Jun 7, 2016

KiChjang commented Jun 7, 2016

davideGiovannini commented Jun 7, 2016

KiChjang commented Jun 7, 2016

paulrouget commented Jun 9, 2016

jdm commented Jun 9, 2016

paulrouget commented Jun 9, 2016

jdm commented Jun 9, 2016

davideGiovannini commented Jun 9, 2016 •

edited by jdm

paulrouget commented Jun 9, 2016

highfive commented Jun 10, 2016

KiChjang commented Jun 13, 2016

nox commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

paulrouget commented Jun 20, 2016

davideGiovannini commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

cbrewster commented Jun 25, 2016

bors-servo commented Jun 25, 2016

bors-servo commented Jun 25, 2016

servo / servo

Join GitHub today

Fixed issue #11651 (Do not fire a blur event when calling .focus() on… #11665

Fixed issue #11651 (Do not fire a blur event when calling .focus() on… #11665

Conversation

davideGiovannini commented Jun 7, 2016 • edited by larsbergstrom

highfive commented Jun 7, 2016

highfive commented Jun 7, 2016

highfive commented Jun 7, 2016

KiChjang commented Jun 7, 2016

davideGiovannini commented Jun 7, 2016

KiChjang commented Jun 7, 2016

paulrouget commented Jun 9, 2016

jdm commented Jun 9, 2016

paulrouget commented Jun 9, 2016

jdm commented Jun 9, 2016

davideGiovannini commented Jun 9, 2016 • edited by jdm

paulrouget commented Jun 9, 2016

highfive commented Jun 10, 2016

KiChjang commented Jun 13, 2016

nox commented Jun 20, 2016

bors-servo commented Jun 20, 2016

bors-servo commented Jun 20, 2016

paulrouget commented Jun 20, 2016

davideGiovannini commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

KiChjang commented Jun 24, 2016

bors-servo commented Jun 24, 2016

bors-servo commented Jun 24, 2016

cbrewster commented Jun 25, 2016

bors-servo commented Jun 25, 2016

bors-servo commented Jun 25, 2016

davideGiovannini commented Jun 7, 2016 •

edited by larsbergstrom

davideGiovannini commented Jun 9, 2016 •

edited by jdm