Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Amend test_designMode to accept either empty string or br tag #18470

Merged
merged 2 commits into from Oct 22, 2019
Merged

Amend test_designMode to accept either empty string or br tag #18470

merged 2 commits into from Oct 22, 2019

Conversation

julianrkung
Copy link
Contributor

This pull request is a continuation of this merged and then reverted pull request.

The innerHTML property should be an empty string according to the W3C spec here: element clear.

The clear should follow the "To clear a content editable element" subroutine, which states:

Set element’s innerHTML IDL attribute to an empty string.

In the reverted PR,I replaced the <br> tag with an empty string. This PR, however, accepts both. This change is necessary because, according to the spec, the element should return an empty string for its innerHTML after its contents are cleared, so the test should be able to pass if the element's innerHTML is an empty string (correct behavior with respect to the spec).

In the closed PR linked above, it's said that all Firefox, Chrome, and Safari show a <br> tag for empty content-editable elements in developer tools. I was able to reproduce this in Firefox but was unable to reproduce it in Chrome (Chrome gave the empty string "" whereas Firefox gave the <br> tag). I did not attempt reproducing in Safari.

@julianrkung julianrkung mentioned this pull request Aug 15, 2019
Merged
@whimboo
Copy link
Contributor

whimboo commented Aug 15, 2019

@julianrkung can you please explain how you tested this with other browsers and which version of Chrome you were using? In my case I tested with a data URL and Chrome 76.0.3809.100.

@julianrkung
Copy link
Contributor Author

@whimboo I appreciate the quick reply!

Chrome version: Version 76.0.3809.100 (Official Build) (64-bit)
Test:
1)Created HTML file with the following contents:
<html><body>hello</body></html>
2) Navigated to it in chrome: file:///usr/path/to/html_file.html
3) I did the following commands in the devtools console (similar to what's in the WPT test)
>> a) document.designMode = "on"
>> b) body = document.getElementByTagName("body")[0]
>> c) body.innerHTML (this was just to check what it was. this returned " hello " with some line breaks)
>> d) body.innerHTML = ""
>> e) body.innerHTML (this returned "")

I did the same procedure for Firefox and got "<br>" at step e.
For Firefox I am using 60.8.0esr (64-bit)

@whimboo
Copy link
Contributor

whimboo commented Aug 16, 2019

Thanks! So I did a mistake by manually removing the content and not doing it via innerHTML. Sorry for that. But while testing this out I found some other interesting details, and those are currently under disussion on https://bugzilla.mozilla.org/show_bug.cgi?id=1573801.

As such lets get clarification first what the HTML spec (and not the WebDriver one) recommends under such a situation.

@andreastt andreastt removed their request for review August 16, 2019 09:32
@andreastt
Copy link
Member

The conclusion from https://bugzilla.mozilla.org/show_bug.cgi?id=1573801 so far is that the behaviour of contenteditable is not defined and that both behaviours is currently acceptable, e.g. the WebDriver conformance tests should accept both <br> and an empty string.

There is interest from Mozilla’s side to be compatible with Chrome’s behaviour, and there are two things that makes this challenging:

  1. the absense of a specification;
  2. and an underlying bug where if we were to remove the <br> node insertion, the parent element would collapse.

It’s my understanding that 1 is not a blocker.

@whimboo
Copy link
Contributor

whimboo commented Aug 20, 2019

@andreastt I assume this would also require a WebDriver spec change before it can be pushed?

@andreastt
Copy link
Member

andreastt commented Aug 20, 2019 •
edited

@whimboo I don’t think so?

The spec says to set the editing host’s innerHTML IDL attribute to an empty string. At present this produces different results in Chrome and Firefox, both of which are acceptable, which is why the test should be updated to tolerate both outcomes.

Am I missing something?

@whimboo
Copy link
Contributor

whimboo commented Aug 21, 2019

Ok, so the spec doesn't tell anything about innerHTML state after it has been set to an empty element. In that case it is indeed fine.

With the current behavior in Firefox we would just not bail out already in step 1, but always try to clear the element, even it has been cleared before. Which also means additional focus and blur events will be fired. For the latter I actually miss a test, which I would also expect to see failing for Firefox when calling clear multiple times on the same element.

@mjzffr mjzffr removed their request for review August 27, 2019 15:18
@JohnChen0
Copy link
Contributor

Could this PR be merged now? Thanks.

@Hexcles
Copy link
Member

Hexcles commented Oct 22, 2019

I see Andreas has approved, so I'm merging it.

@Hexcles Hexcles merged commit 2f293bc into web-platform-tests:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreastt andreastt andreastt approved these changes

@AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

@shs96c shs96c Awaiting requested review from shs96c

@whimboo whimboo Awaiting requested review from whimboo

Assignees

@andreastt andreastt

Labels
webdriver wg-testing_browser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@julianrkung @whimboo @andreastt @JohnChen0 @Hexcles @wpt-pr-bot @moz-wptsync-bot