1437716 - Web Platform Tests for focus are failing

Status: NEW

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

Description

[Sorin Davidoi]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180212220112

Steps to reproduce:

Ran the Web Platform Tests related to focus (http://w3c-test.org/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html)


Actual results:

Firefox fails 6/7 tests.


Expected results:

Firefox passes all tests.

Comment 1

[Sorin Davidoi]

Attachment: fix(dom/base/Element): Clear focus when the active element is hidden

This fixes a failing Web Platform Test (https://github.com/w3c/web-platform-tests/blob/009111410a1099e85d4027a679985975757ceb4d/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html#L36). Wasn't sure where exactly to place this logic - it makes sense to me to be done before the attribute changed callback is called.

Review commit: https://reviewboard.mozilla.org/r/219686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219686/

Comment 2

[Sorin Davidoi]

Attachment: fix(dom/html/nsGenericHTMLElement): Clear focused when the active element is disabled

Fixes there failing Web Platform Tests:
 - https://github.com/w3c/web-platform-tests/blob/009111410a1099e85d4027a679985975757ceb4d/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html#L24
 - https://github.com/w3c/web-platform-tests/blob/009111410a1099e85d4027a679985975757ceb4d/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html#L62
 - https://github.com/w3c/web-platform-tests/blob/009111410a1099e85d4027a679985975757ceb4d/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html#L74

Review commit: https://reviewboard.mozilla.org/r/219688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219688/

Comment 3

[Sorin Davidoi]

Attachment: fix(dom/html/nsGenericHTMLElement): Clear focus when a contentEditable is disabled on the active element

Fixes a failing Web Platform Test (https://github.com/w3c/web-platform-tests/blob/009111410a1099e85d4027a679985975757ceb4d/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html#L99).

Review commit: https://reviewboard.mozilla.org/r/219690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219690/

Comment 4

[Sorin Davidoi]

Attachment: fix(dom/base/Element): Clear focus when tabIndex is unset from the active element

Fixes a failing Web Platform Test (https://github.com/w3c/web-platform-tests/blob/009111410a1099e85d4027a679985975757ceb4d/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html#L86).

Review commit: https://reviewboard.mozilla.org/r/219692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219692/

Comment 5

[Sorin Davidoi]

Attachment: refactor(dom/base/Element): Use Blur() when the element is hidden

Review commit: https://reviewboard.mozilla.org/r/219694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219694/

Comment 6

[Sorin Davidoi]

Attachment: refactor(dom/base/Element): Use Blur() when the tabIndex is removed

Review commit: https://reviewboard.mozilla.org/r/219696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219696/

Comment 7

[Sorin Davidoi]

Attachment: refactor(dom/html/nsGenericHTMLElement): Use Blur() when the element is disabled

Review commit: https://reviewboard.mozilla.org/r/219698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219698/

Comment 8

[Sorin Davidoi]

Attachment: fix(dom/html/nsGenericHTMLElement): Fix bluring when a contentEditable is disabled

Review commit: https://reviewboard.mozilla.org/r/219700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219700/

Comment 9

[Sorin Davidoi]

This also fixes https://bugzilla.mozilla.org/show_bug.cgi?id=999619, where I've attached the patches the first time around.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I've been told this might also fall in Neil's area.

Comment 11

[Neil Deakin (away until Sept 6)]

The main issue here is that there are lots of other ways to hide an element that don't involve setting attributes, and while the cases here should be handled, there are lots of others that aren't. 

Note also that the hidden and tabindex attributes don't have any meaning on elements in different namespaces, so shouldn't be treated as meaningful.

Comment 12

[Sorin Davidoi]

(In reply to Neil Deakin from comment #11)
> The main issue here is that there are lots of other ways to hide an element
> that don't involve setting attributes, and while the cases here should be
> handled, there are lots of others that aren't. 

Is there a centralized place where this can be hooked into?

Comment 13

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

layout, basically CSSFrameConstructor should know whether some element loses its nsIFrame object.

Comment 14

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

Sorin, are you possibly updating the patches or should I review them now or what?
And sorry about delay.

Comment 15

[Sorin Davidoi]

I will try to update them.

Comment 16

[Sorin Davidoi]

(In reply to Olli Pettay [:smaug] from comment #13)
> layout, basically CSSFrameConstructor should know whether some element loses
> its nsIFrame object.

I tried poking around but I can't identify where this happens.

Comment 17

[Sorin Davidoi]

I think this can be set to unassigned since I'm not working on it anymore.