Closed Bug 1639237 Opened 5 years ago Closed 5 years ago

table cellspacing attribute stops working

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Version:
76 Branch
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1636516

People

(Reporter: andreas, Unassigned)

References

Details

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.138 Safari/537.36

Steps to reproduce:

Works correctly in Firefox v56, broken in Firefox v57, also broken still in v76.

This issues shows up deep-down in our large intricate web editor, there are nodes being created off-screen and then assembled together, also iframes are involved. So it's extremely challenging to boil it down to a small reproducible test case that I'm allowed to share. So I'm hoping that I can explain the problem in detail here and maybe the problem will be reasonably self-evident on your end. My attempts at reproducing it from scratch hasn't worked out so far, so I haven't yet been able to figure out what it is that causes the bug.

We're well aware of the fact that cellspacing and cellpadding are deprecated, but in this instance we are editing HTML meant for email where cellspacing is the right choice.

Actual results:

The structure in question is simple <table cellspacing="0" cellpadding="0"><tbody><tr><td>. However, when rendered in our editor cellspacing isn't actually rendered on screen as 0, but with the default value and it becomes visibly offset from the surrounding elements, and no matter how I change the attribute in Firefox devtools nothing happens. If I change cellpadding the change is reflected correctly. There are no inherited CSS-styles and devtools does not show any other computed values that could interfere.

If I refresh that particular node structure in the editor it suddenly starts behaving correctly and when I change cellspacing on that refreshed node structure it is correctly reflected in the on-screen rendering. So it seems the bug is triggered by a specific circumstance, and not strictly related to the initialization of the table itself.

I've tried prodding cellspacing for the table in the console, setting it, getting it, removing and setting it again also. The operations themselves behave correctly but the change is not reflected in on-screen rendering. Whereas doing the same with cellpadding reflects correctly in on-screen rendering.

If I use "edit html" on the structure in devtools and add a space at the front to make Firefox recreate the structure, the problem also goes away.

In our real-world example, there are two nested such examples, and both of them exhibit the exact same broken behavior at the same time. Both cellspacing and cellpadding are set in the same way at same time.

Setting style border-spacing on a "corrupted node" renders correctly. Afterwards removing border-spacing makes the broken cellspacing behavior return.

So it seems as if somehow the cellspacing attribute/logic can become "corrupted" on a node in some sense, during an edge-case. Whereas the same bug never occurs for cellpadding. Despite both being handled in the exact same way by a generic routine.

Full disclosure, the initial table has style="width: 100%; position: relative; top: 0px; z-index: 1;" also set, but removing that in devtools seems to make no difference for this bug. So I omitted that just for simplicity (but just putting here in-case there's a weird connection).

I sincerely hope this is enough to root out the cause of the bug, and then creating a repro when the internal problem is understood.

While further investing this, I've come upon a way to "uncorrupt a corrupted node". If I remove the attribute, then add the attribute back with a non-0 value. Cellspacing for the node now behaves correctly and I can even set it to 0 and it will render correctly. If I instead remove the attribute and then add the attribute back with value 0, then the node will still be in a "corrupted state".

Further to add to the above, if I initialize the nodes with e.g. cellspacing=10, they still end up corrupted as previously. If I remove the attribute and add it back, I have to not set it to 10 again in order to uncorrupt it. If I set it to 10 it stays corrupted. So it seems when re-adding the attribute it must not be the same as the initial value. Further more. If I remove the attribute and add it back with the initial value such that it stays corrupted, it seems then that the above solution for uncorrupting it no longer work and it stays permanently corrupted.

Component: Untriaged → Layout: Tables
Product: Firefox → Core

Can you attach a test-case that reproduces the issue so that we can reproduce this?

Flags: needinfo?(andreas)

Emilio I wish I could, but the codepath leading up to the problem in question is very complex and involves many different algorithms, it seems very daunting to narrow it down to an isolated repro. Iframes, elements being rendered off-screen in various ways and then assembled. I've been trying to reproduce it from scratch by trying to emulate what the code does, but haven't been able to yet. So I was hoping that given the clear nature of being connected to cellspacing and the attribute itself seemingly not replicating to the internal state correctly or something would be enough to give you a lead on what could be causing it, and then I/we could try to create a repro with that knowledge. In combination with the knowledge that the bug was introduced in Firefox v57.

I'm not sure if it's of any help, but I could demonstrate the bug via remote screen for instance or similar. But due to the sensitive nature of the project I cannot share any significant part of the code.

To further add to the exploration above:

This is a very peculiar problem, if I defer adding the cellspacing and cellpadding attribute with requestAnimationFrame, it does not corrupt. If I defer just cellspacing but immediately set cellpadding, it corrupts. It does NOT corrupt if I defer setting cellspacing and never set cellpadding. Immediately setting cellspacing and but never setting cellpadding, still corrupts.

Corrupts, as in the above problem with cellspacing not behaving correctly.

Without a test-case, the other thing you can do is running mozregression to find what caused it.

From the regressing change and the description maybe we could figure out what's going on.

Also, worth a shot testing on Firefox Nightly, just in case.

Ah, mozregression was excellent!
It narrowed it down to; 2017-09-05 is GOOD, 2017-09-06 is BAD. However, bisection of revisions seems broken. It spews out to of warnings as shown below and then errors out. Not sure if these dates are too far back or if you know how to workaround this issue?

2020-05-19T22:28:16.547000: WARNING : Skipping build 50857982881a: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.50857982881ae7803ceb438fee90650a282f7f05.firefox.win64-opt'
2020-05-19T22:28:16.601000: DEBUG : nothing found via route 'gecko.v2.mozilla-central.revision.fd1ab37308c5cf0e1fd2471708f53d8e6ddc69aa.firefox.win64-pgo'
2020-05-19T22:28:16.602000: WARNING : Skipping build fd1ab37308c5: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.fd1ab37308c5cf0e1fd2471708f53d8e6ddc69aa.firefox.win64-opt'
2020-05-19T22:28:16.619000: DEBUG : nothing found via route 'gecko.v2.mozilla-central.revision.dd75dcec7da162d8ceaaf0883e0e7561bd772992.firefox.win64-pgo'
2020-05-19T22:28:16.620000: WARNING : Skipping build dd75dcec7da1: Unable to find build info using the taskcluster route 'gecko.v2.mozilla-central.shippable.revision.dd75dcec7da162d8ceaaf0883e0e7561bd772992.firefox.win64-opt'

Haha, I had tried Firefox beta before and it was still there, but it seems to actually be fixed in nightly!
That's an interesting timing coincidence given how long ago it was introduced, it's almost suspicious. Could it be that nightly is built differently such that the error doesn't appear there?

Can you paste the pushlog link that you got from the run in comment 7? The warnings are probably because the builds are a bit old and we haven't kept them around.

(In reply to Andreas Svensson from comment #8)

Haha, I had tried Firefox beta before and it was still there, but it seems to actually be fixed in nightly!
That's an interesting timing coincidence given how long ago it was introduced, it's almost suspicious. Could it be that nightly is built differently such that the error doesn't appear there?

That is interesting, I wonder what the difference may be... Maybe try mozregression --bad 76 --find-fix to figure out what fixed it? Mozregression only runs nightly builds, so it seems unlikely to be a build-related thing...

Found the fix. I wonder what the exact mechanism was though because it doesn't seem directly connected, but indirectly.
Thanks so much for your help!

2020-05-19T22:42:35.025000: INFO : Narrowed integration regression window from [928a5891, 7365c2be] (4 builds) to [614e5285, 7365c2be] (2 builds) (~1 steps left)
2020-05-19T22:42:35.030000: DEBUG : Starting merge handling...
2020-05-19T22:42:35.030000: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=7365c2beddabe67206e9368f903e72878490d4dc&full=1
2020-05-19T22:42:36.015000: DEBUG : Found commit message:
Bug 1636516 - When moving an element to another document, don't drop nsMappedAttributes from its sheet. r=edgar

That is, drop the reference to the sheet pointer. GetModifiableMapped
doesn't ever return anything that's on the hash. But trying to
DropMappedAttributes it will remove not that instance, but any other
instance that's already in the hash and matches it.

ThisDropSheetReference is always done in MakeMappedUnique if relevant.

On the original test-case it was about the -moz-user-modify property
that contenteditable maps, but it reproduces with any mapped attribute
like hidden, so I used that for WPT.

Differential Revision: https://phabricator.services.mozilla.com/D74498

2020-05-19T22:42:36.016000: DEBUG : Did not find a branch, checking all integration branches
2020-05-19T22:42:36.017000: INFO : The bisection is done.
2020-05-19T22:42:36.019000: INFO : Stopped

Flags: needinfo?(andreas)

Regarding comment 7, I'm not sure what you're asking for exactly. If I start the bisect at the given dates. This is the full output in the console.
https://gist.github.com/syranide/3a74399a0c8e0556e0a59e54e4d0e215

If you can clarify what you're after I can get it for you.

LOL, so I fixed it just a little bit more than a week ago, nice :)

Ok, that makes some sense. It is not related to cellspacing in particular but about all mapped attributes in general when you adopt nodes across documents before elements are ever styled.

Alright then, let's close this as a dupe. Thank you! As a workaround, you can use the border-spacing CSS property instead of the cellspacing attribute.

Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE

Haha yeah I saw that :D

Amazing coincidence, and it probably means that I should look at why some nodes are getting transferred to another document, they shouldn't.

Thank you again.

Np, thanks for filing! :)

Just to add the final nail in the coffin for this bug. I tracked down a node being incorrectly created with the wrong document and thus being transferred. With that fixed the broken behavior went away.

Don't want to derail a bug thread, but it seems like implicit transferring of document node ownership is one of those things that very few people know about, is easy to mess up and comes with a performance cost if you do. Have you considered an optional devtools warning or similar for it? Don't feel obliged to answer, just putting it out there :)

Hmm, I think that's a good idea. Though it's hard to know when it's intentional vs. not... For example it seems it'd be pretty easy to trigger such a warning when using <template> or so... Filed bug 1639237 for it anyhow.

See Also: → 1639508
You need to log in before you can comment on or make changes to this bug.