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

Change test_designMode to check for empty string instead of br tag #17809

Merged
merged 1 commit into from Aug 14, 2019

Conversation

@julianrkung
Copy link
Contributor

julianrkung commented Jul 12, 2019

test_designMode incorrectly checks that the innerHTML after an element_clear is a <br> tag. Instead, 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:

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

Nowhere in the spec for element clear is the insertion of a <br> tag mentioned, so rather than accept both answers, to be spec compliant only an empty string should be accepted.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Jul 24, 2019

I think that this looks fine. But @andreastt added that part, so Andreas if you could check that too, we can get this merged.

@julianrkung

This comment has been minimized.

Copy link
Contributor Author

julianrkung commented Aug 1, 2019

Any updates on this? Would liked this pushed through before I forget about it

@whimboo
whimboo approved these changes Aug 2, 2019
Copy link
Contributor

whimboo left a comment

As mentioned in my last commit it looks fine, and as such I will give r+. If @andreastt doesn't respond soon, we should assume its fine and can be merged.

@julianrkung

This comment has been minimized.

Copy link
Contributor Author

julianrkung commented Aug 13, 2019

Would it be possible to merge it now that sufficient time has passed without response?

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Aug 14, 2019

Yes, I think we should finally get this merged. Thanks for pinging.

@whimboo whimboo merged commit 0ca74f4 into web-platform-tests:master Aug 14, 2019
3 of 4 checks passed
3 of 4 checks passed
Azure Pipelines Build #23465 failed
Details
wpt.fyi - firefox[experimental] Firefox results
Details
Taskcluster (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Aug 14, 2019

As it turned out and further discussed on https://bugzilla.mozilla.org/show_bug.cgi?id=1573801, the PR should have not been merged. After the comment of @andreastt on that bug I checked the behavior of Firefox, Chrome, and Safari. All of them show a <br> tag for an empty content-editable element in the developer tools. So chromedriver might do a replacement here?

Could someone please help with a backout of this patch? Thanks

jgraham added a commit that referenced this pull request Aug 14, 2019
…br tag (#17809)"

This reverts commit 0ca74f4.
@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Aug 14, 2019

@julianrkung Please feel free to file an issue for further discussion. This PR cannot be reopened.

whimboo added a commit that referenced this pull request Aug 14, 2019
…br tag (#17809)" (#18421)

This reverts commit 0ca74f4.
@julianrkung

This comment has been minimized.

Copy link
Contributor Author

julianrkung commented Aug 15, 2019

I opened another PR that accepts both <br> and the empty string "". I wasn't able to reproduce the <br> tag showing for empty content-editable elements in Chrome, but was able to reproduce it in Firefox. I did not try to reproduce it in Safari.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.