Bug 112207 – overflow:ellipsis element not getting relayouted when the contents of it's float sibling changes

• Component: Layout and Rendering
• Status: NEW

[Ojan Vafai]

Created attachment 192832 [details]
testcase

See test case

[Levi Weintraub]

I can't get this to repro.

[Ojan Vafai]

This happens for me on ToT. Not sure why it sometimes doesn't for some folk.

[Ojan Vafai]

Created attachment 193003 [details]
better test case

This test case looks to reproduce more reliably.

[Elliott Sprehn]

(In reply to comment #3)
> Created an attachment (id=193003) [details]
> better test case
> 
> This test case looks to reproduce more reliably.

So I tracked down why this doesn't seem to repro for some people. AdBlock (or other extensions) cause extra recalcStyles.

[Ojan Vafai]

So, the problem here is that we don't mark blocks that avoided a float for layout when the float shrinks it's height. It seems to me that we need to keep a bit as to whether a block avoided floats and always mark those blocks as needing layout in layoutBlockChild. We can't just keep the previous float logical bottom to figure it out later because the height of one of the block's parents could change in a way that the block is below the previous float logical bottom.

[Ojan Vafai]

Created attachment 193184 [details]
Patch

[Robert Hogan]

I'm pretty sure adding a bit to RenderBlock isn't the way to go here. That's very much a last resort, no?

[Ojan Vafai]

(In reply to comment #7)
> I'm pretty sure adding a bit to RenderBlock isn't the way to go here. That's very much a last resort, no?

It's definitely not ideal, but I don't think it's the end of the world either. We have a lot of bits to spare at the moment, i.e. how many bits does lineheight really need? Certainly less than 26. :)

I couldn't figure out another way to tell if a block *previously* had intruding floats, but now doesn't. Maybe we could, figure out what it's width would be without intruding floats and if that's not it's current width, then we mark descendants as needing layout. The downside there is that we'd walk all the descendants and mark them as needing layout if the width of the block changed even if there were never floats involved. I expect that would have bad performance implications (e.g. resizing the window would do an n^2 walk of the rendertree marking all the blocks as needing layout).

Definitely open to suggestions on how to do this better if you have any ideas.

[Emil A Eklund]

(In reply to comment #8)
> (In reply to comment #7)
> > I'm pretty sure adding a bit to RenderBlock isn't the way to go here. That's very much a last resort, no?
> 
> It's definitely not ideal, but I don't think it's the end of the world either. We have a lot of bits to spare at the moment, i.e. how many bits does lineheight really need? Certainly less than 26. :)

For now we need at least 24 bits to allow for a full range of values, at some point we want to increase the precision of line-height (for high-dpi devices) though and we'll need at least one or two more bits for that.

[Dave Hyatt]

We discussed this on IRC, and I'd like to see a solution that really checks the old floating objects set rather than this code that relies on looking at logical bottom positions, etc.

I think that layout, instead of marking all descendants, should be more like pagination layout and do a check at each level.

If a child does not need a layout, then what you can do is factor out the code from clearFloats that rebuilds your floating object set into a function that gets you the set without storing it as a member variable.

Once you have that new set, you can compare with the current set on the child and see if anything has changed. This should also include the positions of any floats relative to that child (i.e., where the floats are in that child's coordinate space).

Obviously you would only do this if the child does not avoid floats.

You would then mark that child as needing layout if the floating objects have changed in some way.

This should be much cleaner than the current code and should be more narrow in scope, i.e., fewer objects should be marked. It's also nice in that you only apply the check at each level rather than doing a descendant crawl the minute any child needed a relayout.

[Dave Hyatt]

You'd probably also want to pass that new set you built in so that you don't then rebuild it a second time. :)

[Ojan Vafai]

I've gotten pretty far with a patch for this. A part I'm getting a bit stuck on is what to do for children that to avoid floats. We don't want to unconditionally always lay them out, but we don't have any data on whether we previously shrank the block to avoid floats. Presumably, we don't even want to unconditionally lay them out even if they did shrink to avoid floats because the floats in question may not have changed.

Any suggestions how to handle this case? I suppose clearFloats could return a bool about whether the floats in the parent have changed, and if so, then we set relayoutChildren to true?

[Ahmad Saleem]

Using the attached test case, in Safari 16.1, I get both container as same and matching with other browsers (Chrome Canary 109 and Firefox Nightly 108) but it seems that Safari Technology Preview 109 has regressed this and in this test case, now the containers don't match each other.

Just wanted to share results. Thanks!

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #13)
> Using the attached test case, in Safari 16.1, I get both container as same
> and matching with other browsers (Chrome Canary 109 and Firefox Nightly 108)
> but it seems that Safari Technology Preview 109 has regressed this and in
> this test case, now the containers don't match each other.
> 
> Just wanted to share results. Thanks!

This seems to be broken in Safari 16.3 as well and WebKit ToT.

So something between Safari 16.1 and 16.3 caused it to regress.

[Radar WebKit Bug Importer]

<rdar://problem/130327169>