Bug 220934

Summary: Table layout issue when cells change from display:block to display:table-cell
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: TablesAssignee: Cameron McCormack (:heycam) <heycam>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, esprehn+autocc, ews-watchlist, glenn, heycam, kondapallykalyan, pdr, sam, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 223490    
Bug Blocks:    
Attachments:
Description Flags
Testcase
none
Patch
none
patch with dependencies for EWS
ews-feeder: commit-queue-
Patch
none
patch with dependencies for EWS
none
Patch none

Description Simon Fraser (smfr) 2021-01-25 10:57:51 PST
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.
Comment 2 Radar WebKit Bug Importer 2021-01-25 10:58:28 PST
<rdar://problem/73578735>
Comment 3 Cameron McCormack (:heycam) 2021-03-18 21:34:45 PDT
test262.report is not loading for me, but my forthcoming patch does fix the attached testcase.
Comment 4 Cameron McCormack (:heycam) 2021-03-18 22:07:05 PDT
Created attachment 423691 [details]
Patch
Comment 5 Cameron McCormack (:heycam) 2021-03-18 22:33:37 PDT
Created attachment 423693 [details]
patch with dependencies for EWS
Comment 6 Cameron McCormack (:heycam) 2021-03-19 14:27:32 PDT
Created attachment 423779 [details]
Patch
Comment 7 Cameron McCormack (:heycam) 2021-03-19 14:29:10 PDT
Created attachment 423780 [details]
patch with dependencies for EWS
Comment 8 Sam Weinig 2021-03-21 10:42:16 PDT
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.
Comment 9 Cameron McCormack (:heycam) 2021-03-21 13:18:50 PDT
(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
Comment 10 Cameron McCormack (:heycam) 2021-03-21 13:51:45 PDT
(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;
  }
Comment 11 zalan 2021-03-21 17:16:11 PDT
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.
Comment 12 Sam Weinig 2021-03-21 18:08:15 PDT
(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.
Comment 13 Cameron McCormack (:heycam) 2021-03-22 14:09:59 PDT
Created attachment 423936 [details]
Patch
Comment 14 EWS 2021-03-25 14:45:10 PDT
Committed r275059: <https://commits.webkit.org/r275059>

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