Bug 220934 – Table layout issue when cells change from display:block to display:table-cell

• Component: Tables
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 418307 [details]
Testcase

The attached testcase shows a table layout bug where resizing the window from wide to narrow (triggering the media query) causes bad table layout; fixed on reload.

[Simon Fraser (smfr)]

Originally seen on https://test262.report/?engines=chakra%2Cjavascriptcore%2Cspidermonkey%2Cv8%2Cxs%2Cqjs%2Cengine262

[Radar WebKit Bug Importer]

<rdar://problem/73578735>

[Cameron McCormack (:heycam)]

test262.report is not loading for me, but my forthcoming patch does fix the attached testcase.

[Cameron McCormack (:heycam)]

Created attachment 423691 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 423693 [details]
patch with dependencies for EWS

[Cameron McCormack (:heycam)]

Created attachment 423779 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 423780 [details]
patch with dependencies for EWS

[Sam Weinig]

Comment on attachment 423779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423779&action=review

> Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:842
> +    };
> +    collapseAndDestroyAnonymousSiblings();

I will let someone capable do the real review, but some style nits:

you can merge this into one line as just:

}();

I'm not sure why the lambda above, clearFloatsAndOutOfFlowPositionedObjects, doesn't do that, but if you could fix it as well that would be good.

> Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp:250
> +template <typename Parent, typename Child>

No space after 'template' here.

> Source/WebCore/rendering/updating/RenderTreeBuilderTable.h:59
> +    template <typename Parent, typename Child>

No space after 'template' here.

[Cameron McCormack (:heycam)]

(In reply to Sam Weinig from comment #8)
> > Source/WebCore/rendering/updating/RenderTreeBuilderTable.cpp:250
> > +template <typename Parent, typename Child>
> 
> No space after 'template' here.

No space wins (only just!) in the codebase:

$ git grep 'template <' | wc -l ; git grep 'template<' | wc -l
   13013
   13057

but it's not what .clang-format says:

$ grep SpaceAfterTemplate .clang-format
SpaceAfterTemplateKeyword: true

[Cameron McCormack (:heycam)]

(In reply to Sam Weinig from comment #8)
> > Source/WebCore/rendering/updating/RenderTreeBuilder.cpp:842
> > +    };
> > +    collapseAndDestroyAnonymousSiblings();
> 
> I will let someone capable do the real review, but some style nits:
> 
> you can merge this into one line as just:
> 
> }();
> 
> I'm not sure why the lambda above, clearFloatsAndOutOfFlowPositionedObjects,
> doesn't do that, but if you could fix it as well that would be good.

I think there's some advantage to naming the lambdas here, to make it clear what they're doing, although perhaps it's obvious from their contents.

I only found two other instances of immediately executed lambda expressions where the return type is void, in CSSPropertyParseHelpers, and for those it's pretty clear what's going on.

If we're getting rid of their names, then clearFloatsAndOutOfFlowPositionedObjects could simply become an if statement.  collapseAndDestroyAnonymousSiblings could too, I guess, but I slightly like the more spaced out:

  if (of this type) {
    do this;
    return;
  }

  if (of that type) {
    do that;
    return;
  }

than:

  if (of this type) {
    do this;
  } else if (of that type) {
    do that;
  }

[alan]

I tend to use this pattern. I don't have a strong preference over "auto foo = [&] { }();" I find it more readable but :shrugs:

>If we're getting rid of their names, then clearFloatsAndOutOfFlowPositionedObjects
I would not do that as their names act as "sticky" comments and they help code readability.

[Sam Weinig]

(In reply to zalan from comment #11)
> I tend to use this pattern. I don't have a strong preference over "auto foo
> = [&] { }();" I find it more readable but :shrugs:
> 
> >If we're getting rid of their names, then clearFloatsAndOutOfFlowPositionedObjects
> I would not do that as their names act as "sticky" comments and they help
> code readability.

My bad, I didn't realize this was idiomatic. Please, ignore.

[Cameron McCormack (:heycam)]

Created attachment 423936 [details]
Patch

[EWS]

Committed r275059: <https://commits.webkit.org/r275059>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423936 [details].