Closed Bug 1573801 Opened 5 years ago Closed 2 years ago

Perma /webdriver/tests/element_clear/clear.py | test_designmode - AssertionError: assert '<br>' == ''

Categories

(Remote Protocol :: Marionette, defect, P5)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WORKSFORME

People

(Reporter: intermittent-bug-filer, Unassigned)

References

(Depends on 1 open bug)

Details

(4 keywords)

Filed by: hskupin [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer.html#?job_id=259553433&repo=try
Full log: https://queue.taskcluster.net/v1/task/E8h06VnEThaonCF7NlyEdw/runs/0/artifacts/public/logs/live_backing.log

This is based on the merge from bug 1565646, and would need a fix in Marionette which simply replaces the <br> with an empty string. For now I assume this merge commit will mark the test as expected fail, so lets wait until that landed on central.

Summary: Intermittent /webdriver/tests/element_clear/clear.py | test_designmode - AssertionError: assert '<br>' == '' → Perma /webdriver/tests/element_clear/clear.py | test_designmode - AssertionError: assert '<br>' == ''

Marionette sets the innerHTML of the editing host to an empty string:
https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/testing/marionette/interaction.js#356

My guess is that this problem is at the DOM level,
where no content implies <br> in Gecko.

I'm sorry that I merged this PR, and as it looks like it's not only Firefox adding a <br> when clearing the editable content but also Chrome, and Safari. I would say we get the PR (https://github.com/web-platform-tests/wpt/pull/17809) backed-out?

I don't have the repo so I cannot do it my own. Andreas, could you help with it please?

Flags: needinfo?(ato)

So if I understand correctly, you are saying this is a spec issue?

(I see the patch got reverted in https://github.com/web-platform-tests/wpt/pull/18421,
so clearing my needinfo.)

Flags: needinfo?(ato) → needinfo?(hskupin)

Maybe. It would need further investigation. There is https://github.com/web-platform-tests/wpt/pull/18470 now.

Flags: needinfo?(hskupin)

Once bug 1573878 gets merged it will be no longer a problem.

Depends on: 1573878

Actually I've made a mistake when checking this with Chrome and Safari. I cleared the contenteditable element by hand, and not by setting its innerHTML value to an empty string. :/ When doing correctly Chrome indeed doesn't return a <br>, but ``.

Test case: data:text/html;content,<html><body contentEditable=true>foo</body></html>

Steps:

  1. Open the dev tools inspector and select Use in Console for the body element
  2. Run temp0.innerHTML = ""

As you can see the content got replaced with <br>.

Interestingly there is no <br> node added when running the same steps with an inline element:

Test case: data:text/html;content,<html><body><span contentEditable=true>foo</span</body></html>

Olli, is there a spec which describes the correct behavior? Is it Firefox which is broken here, or all the other browsers?

Flags: needinfo?(bugs)

Unfortunately there is no spec for contenteditable. So browsers do whatever they happen to do.

Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #7)

Unfortunately there is no spec for contenteditable. So browsers do whatever they happen to do.

Given that all other browsers [1] keep the empty innerHTML value, and only Firefox replaces it with <br>, shouldn't we just drop our behavior and also keep it an empty value? It would help us for cross-browser compatibility even with no spec for contenteditable being present yet?

[1] https://wpt.fyi/results/webdriver/tests/element_clear/clear.py?label=master&label=experimental&product=chrome&product=firefox&product=safari&product=edge

Flags: needinfo?(bugs)

Maybe, or probably, but that means someone reverse-engineering what other browsers do with contentEditable and write a spec. So far that hasn't happened - it isn't exactly a trivial task.

Flags: needinfo?(bugs)

Ok, so I wonder what we should do with this particular test. Maybe we should get the original patch re-landed, and mark the test as expected fail? Anything else would require a WebDriver spec change, or munging with the value, which means we cannot correctly detect an empty innerHTML value vs. a real line break. Andreas, what do you think?

Flags: needinfo?(ato)

We shouldn’t re-land the original patch, because that just replaces
<br> with an empty string. Going by what smaug said earlier, the
WebDriver conformance test suite needs to accept both behaviours
because they’re equally correct.

This means we should probably accept
https://github.com/web-platform-tests/wpt/pull/18470 as a substitute.

Flags: needinfo?(ato)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #8)

(In reply to Olli Pettay [:smaug] from comment #7)

Unfortunately there is no spec for contenteditable. So browsers do whatever they happen to do.

Given that all other browsers [1] keep the empty innerHTML value, and only Firefox replaces it with <br>, shouldn't we just drop our behavior and also keep it an empty value?

We probably should, in fact. This node used to be created internally (here) because we were unable to place carets inside empty block elements which would have zero height, as the height of the caret used to depend on its containing element. However we fixed that problem many years ago, and ever since then we were not aware of other use cases for this node (besides possibly code somewhere in the tree which depends on its existence.) When I owned the Editor module I had an item on my long term wish list to get rid of this element.

I believe that in order to fix this bug, we should fix Gecko to remove the padding BR element for empty editors...

We have bug 503838 open on this, FWIW.

Well, there is a remaining issue to remove the padding <br> for the empty editor.

  1. Load data:text/html,<div contenteditable>a</div>
  2. Remove the "a"

Chrome keeps one-line height for the <div> so that users can set focus with pointing device. However, if we'll stop creating the padding <br>, the <div> becomes 0 height. See bug 1098151.

I cannot see that with Chrome. When I press backspace once, and then blur the div by clicking somewhere outside the contenteditable the innerHTML property shows as empty via dev tools. Only when I hit Return it will add the new line for sure.

But our specific case here is that directly setting innerHTML to an empty string, results in <br>, which is a behavior no other browser shows. I cannot say how this is related to when the user clears the content manually.

Depends on: better-invisible-br-handling

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(no pain of the broken bone, but the corset blocks me to concentrate) from comment #13)

Well, there is a remaining issue to remove the padding <br> for the empty editor.

  1. Load data:text/html,<div contenteditable>a</div>
  2. Remove the "a"

Chrome keeps one-line height for the <div> so that users can set focus with pointing device. However, if we'll stop creating the padding <br>, the <div> becomes 0 height. See bug 1098151.

OK, this makes sense to me. I wonder if there are any other cases which we also need to consider (e.g. with writing modes, etc...)

The test is no longer marked as failing. So I assume that we can just close the bug.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
Moving bug to Remote Protocol::Marionette component per bug 1815831.
Component: geckodriver → Marionette
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.