Marquee width should not change a fixed width of its container.

Marquee width should not change a fixed width of its container.

Marquee collapses a block layout when the content of marquee is long enough
to overflow the fixed width of its container. It should not contribute to the container's width.
Conversely it should be constrained by its parents. To fix this, we don't accumulate
marquee's width when calculating container block's width.

BUG=459979
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197819

[changseok, 2015-06-23 06:50:02]

shivamidow@gmail.com changed reviewers:
+ eae@chromium.org, leviw@chromium.org, mstensho@opera.com

[changseok, 2015-06-23 06:50:03]

PTAL.

[mstensho (USE GERRIT), 2015-06-23 09:33:31]

Please take a second look at the commit message. It misspells "marquee" once,
and also, this really isn't table-specific. It applies to anything that uses min
preferred logical width (any shrink-to-fit container). And being a composited
layer has nothing to do with this bug.

https://codereview.chromium.org/1200103006/diff/1/LayoutTests/fast/table/tabl...
File LayoutTests/fast/table/table-width-with-marquee.html (right):

https://codereview.chromium.org/1200103006/diff/1/LayoutTests/fast/table/tabl...
LayoutTests/fast/table/table-width-with-marquee.html:6: <marquee>The quick brown
fox jumps over the lazy dog.</marquee>
Can you add some more text? This is barely wider than 200px.

https://codereview.chromium.org/1200103006/diff/1/LayoutTests/fast/table/tabl...
LayoutTests/fast/table/table-width-with-marquee.html:12:
shouldBeEqualToNumber("document.querySelector('td').getBoundingClientRect().width",
196);
Setting cellpadding="0" cellspacing="0" on the table should make it possible to
compare against 200 instead of 196.

Could also avoid tables altogether:

<div style="width:200px;">
    <div id="check-this-element-it-should-be-200px-wide"
style="width:intrinsic;">
         <marquee>........

https://codereview.chromium.org/1200103006/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1200103006/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:2010: if (!styleRef().autoWrap() && layer()
&& isHTMLMarqueeElement(node())) {
What's with the layer() check?

Also not sure why we need the !styleRef().autoWrap() check.
HTMLMarqueeElement.js controls sets the white-space property on the MARQUEE
element (nowrap for horizontal marquees). If the document overrides this to get
a broken marquee, who cares?

https://codereview.chromium.org/1200103006/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:2011: AtomicString direction =
toElement(node())->getAttribute(directionAttr);
I think I'd like to see this logic being moved to HTMLMarqueeElement. Add an
HTMLMarqueeElement::isHorizontalMarquee()?

[changseok, 2015-06-24 06:15:58]

Thanks for all your comments!

> Please take a second look at the commit message. It misspells "marquee" once,
Fixed.

> and also, this really isn't table-specific. It applies to anything that uses
min
> preferred logical width (any shrink-to-fit container). And being a composited
> layer has nothing to do with this bug.
I see.

https://codereview.chromium.org/1200103006/diff/1/LayoutTests/fast/table/tabl...
File LayoutTests/fast/table/table-width-with-marquee.html (right):

https://codereview.chromium.org/1200103006/diff/1/LayoutTests/fast/table/tabl...
LayoutTests/fast/table/table-width-with-marquee.html:6: <marquee>The quick brown
fox jumps over the lazy dog.</marquee>
On 2015/06/23 09:33:30, mstensho wrote:
> Can you add some more text? This is barely wider than 200px.

Done.

https://codereview.chromium.org/1200103006/diff/1/LayoutTests/fast/table/tabl...
LayoutTests/fast/table/table-width-with-marquee.html:12:
shouldBeEqualToNumber("document.querySelector('td').getBoundingClientRect().width",
196);
On 2015/06/23 09:33:30, mstensho wrote:
> Setting cellpadding="0" cellspacing="0" on the table should make it possible
to
> compare against 200 instead of 196.
> 
> Could also avoid tables altogether:
> 
> <div style="width:200px;">
>     <div id="check-this-element-it-should-be-200px-wide"
> style="width:intrinsic;">
>          <marquee>........

O.K. The table is replaced with the two divs

https://codereview.chromium.org/1200103006/diff/1/Source/core/layout/LayoutBl...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1200103006/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:2010: if (!styleRef().autoWrap() && layer()
&& isHTMLMarqueeElement(node())) {
On 2015/06/23 09:33:30, mstensho wrote:
> What's with the layer() check?
This is garbage code. This change is inspired by webkit code. It checks if a
render block is a marquee through RenderLayer so it needs to check a layer
existence first. But we don't need to do because we manipulate the marquee with
a shadow root as you said, the layer has no way to check if a layout object is
marquee or not.

> Also not sure why we need the !styleRef().autoWrap() check.
> HTMLMarqueeElement.js controls sets the white-space property on the MARQUEE
> element (nowrap for horizontal marquees). If the document overrides this to
get
> a broken marquee, who cares?
You're right, That was a redundant check for direction.

https://codereview.chromium.org/1200103006/diff/1/Source/core/layout/LayoutBl...
Source/core/layout/LayoutBlock.cpp:2011: AtomicString direction =
toElement(node())->getAttribute(directionAttr);
On 2015/06/23 09:33:31, mstensho wrote:
> I think I'd like to see this logic being moved to HTMLMarqueeElement. Add an
> HTMLMarqueeElement::isHorizontalMarquee()?

OK

[mstensho (USE GERRIT), 2015-06-24 08:24:37]

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
File LayoutTests/fast/block/marquee-in-fixed-size-container.html (right):

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
LayoutTests/fast/block/marquee-in-fixed-size-container.html:1: <!DOCTYPE html>
I think we need a better file name. What's of interest here is the fact that
we're measuring intrinsic minimum width.

marquee-in-shrink-to-fit-in-fixed-size-container.html or something less ugly if
you can think of something?

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
LayoutTests/fast/block/marquee-in-fixed-size-container.html:4: div {
No need for this. DIVs don't have padding or margin in the UA style sheet.

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
LayoutTests/fast/block/marquee-in-fixed-size-container.html:21:
description("This tests if marquee contributes to the width of its container
having a fixed width size. It should not.");
This isn't quite right. It's not the container of the marquee that has fixed
width: Its container is a shrink-to-fit container, which in turn lives in a
width-constrained container.

https://codereview.chromium.org/1200103006/diff/20001/Source/core/html/HTMLMa...
File Source/core/html/HTMLMarqueeElement.cpp (right):

https://codereview.chromium.org/1200103006/diff/20001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.cpp:77: return direction != "down" &&
direction != "up";
I suddenly started to wonder about the case-sensitive matches here, but I see
that this is in line with the rest of the implementation. <marquee
direction="DoWN"> does indeed become horiontal (while <marquee direction="down">
becomes vertical). Nothing to worry about, then. Weird, though.

https://codereview.chromium.org/1200103006/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1200103006/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:2012: minLogicalWidth = 0;
minLogicalWidth = LayoutUnit() is cheaper (or you'll end up in setValue() and
saturatedSet() for no reason).

[changseok, 2015-06-25 06:41:34]

Thank you for all comments. =)

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
File LayoutTests/fast/block/marquee-in-fixed-size-container.html (right):

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
LayoutTests/fast/block/marquee-in-fixed-size-container.html:1: <!DOCTYPE html>
On 2015/06/24 08:24:37, mstensho wrote:
> I think we need a better file name. What's of interest here is the fact that
> we're measuring intrinsic minimum width.
> 
> marquee-in-shrink-to-fit-in-fixed-size-container.html or something less ugly
if
> you can think of something?

Renamed marquee-width-shrinks-to-fit-in-fixed-size-container.html

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
LayoutTests/fast/block/marquee-in-fixed-size-container.html:4: div {
On 2015/06/24 08:24:37, mstensho wrote:
> No need for this. DIVs don't have padding or margin in the UA style sheet.

Removed.

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
LayoutTests/fast/block/marquee-in-fixed-size-container.html:21:
description("This tests if marquee contributes to the width of its container
having a fixed width size. It should not.");
On 2015/06/24 08:24:37, mstensho wrote:
> This isn't quite right. It's not the container of the marquee that has fixed
> width: Its container is a shrink-to-fit container, which in turn lives in a
> width-constrained container.

How about this?
"This tests if a block having an intrinsic width and a marquee child collapses a
fixed width of its container block. It should not."

https://codereview.chromium.org/1200103006/diff/20001/Source/core/html/HTMLMa...
File Source/core/html/HTMLMarqueeElement.cpp (right):

https://codereview.chromium.org/1200103006/diff/20001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.cpp:77: return direction != "down" &&
direction != "up";
On 2015/06/24 08:24:37, mstensho wrote:
> I suddenly started to wonder about the case-sensitive matches here, but I see
> that this is in line with the rest of the implementation. <marquee
> direction="DoWN"> does indeed become horiontal (while <marquee
direction="down">
> becomes vertical). Nothing to worry about, then. Weird, though.
I think that's a parser issue. Parser presumably ignores the unmatched string so
that we just see the default direction, horizontal left. Blink parser seems
stricter than the others. Gecko, webkit are not case sensitive, but blink is.
Do you think we need to ignore cases here?

https://codereview.chromium.org/1200103006/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1200103006/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:2012: minLogicalWidth = 0;
On 2015/06/24 08:24:37, mstensho wrote:
> minLogicalWidth = LayoutUnit() is cheaper (or you'll end up in setValue() and
> saturatedSet() for no reason).

Done.

[mstensho (USE GERRIT), 2015-06-25 07:46:47]

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
File LayoutTests/fast/block/marquee-in-fixed-size-container.html (right):

https://codereview.chromium.org/1200103006/diff/20001/LayoutTests/fast/block/...
LayoutTests/fast/block/marquee-in-fixed-size-container.html:21:
description("This tests if marquee contributes to the width of its container
having a fixed width size. It should not.");
On 2015/06/25 06:41:34, changseok wrote:
> On 2015/06/24 08:24:37, mstensho wrote:
> > This isn't quite right. It's not the container of the marquee that has fixed
> > width: Its container is a shrink-to-fit container, which in turn lives in a
> > width-constrained container.
> 
> How about this?
> "This tests if a block having an intrinsic width and a marquee child collapses
a
> fixed width of its container block. It should not."

I don't understand the word "collapses" in this context.

How about: "Test that a horizontal marquee doesn't contribute to a container's
intrinsic minimum width"

https://codereview.chromium.org/1200103006/diff/20001/Source/core/html/HTMLMa...
File Source/core/html/HTMLMarqueeElement.cpp (right):

https://codereview.chromium.org/1200103006/diff/20001/Source/core/html/HTMLMa...
Source/core/html/HTMLMarqueeElement.cpp:77: return direction != "down" &&
direction != "up";
On 2015/06/25 06:41:34, changseok wrote:
> On 2015/06/24 08:24:37, mstensho wrote:
> > I suddenly started to wonder about the case-sensitive matches here, but I
see
> > that this is in line with the rest of the implementation. <marquee
> > direction="DoWN"> does indeed become horiontal (while <marquee
> direction="down">
> > becomes vertical). Nothing to worry about, then. Weird, though.
> I think that's a parser issue. Parser presumably ignores the unmatched string
so
> that we just see the default direction, horizontal left. Blink parser seems
> stricter than the others. Gecko, webkit are not case sensitive, but blink is.
> Do you think we need to ignore cases here?

It's weird to be case-sensitive in this case, but your code here is fine. If we
want to fix this, we should do it in a separate CL.

[changseok, 2015-06-25 08:40:32]

On 2015/06/25 07:46:47, mstensho wrote:
> How about: "Test that a horizontal marquee doesn't contribute to a container's
> intrinsic minimum width"
Sounds good. Applied. Thanks.

> It's weird to be case-sensitive in this case, but your code here is fine. If
we
> want to fix this, we should do it in a separate CL.
O.K. I opened a bug to follow this up. crbug.com/504268

[mstensho (USE GERRIT), 2015-06-25 08:44:13]

lgtm

[changseok, 2015-06-25 08:56:55]

The CQ bit was checked by shivamidow@gmail.com

[commit-bot: I haz the power, 2015-06-25 09:01:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1200103006/80001

[commit-bot: I haz the power, 2015-06-25 12:56:02]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197819