1260031 - [float pref width] Search button is pushed down (displays on the 2nd line) on http://thirdworld.nl/

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160326030430
STR:
1. Open http://thirdworld.nl/
2. Look at the search field and search button at the top right of the page

AR:  Search button is located below search field
ER:  Search button should be in the same line

Notes:
Yes, it's been a while since I last visited that site. For now, I'm marking this as "regression" with component "layout:floats" since I haven't looked at the actual CSS. It's OK in IE11 & GoogleChrome

This is regression from 478834. Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ed8935b903edf99d2bf90fdee30467db7d4c014e&tochange=c63a6810b2bb84ada4fe207ec58171a080c0447d

Comment 1

[Alice0775 White]

Attachment: reduced html

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: slightly simpler testcase, with colors

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: slightly simpler testcase, with colors

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the intrinsic width was always incorrect (relative to Chromium, anyway) by being too narrow, both before and after the regression.

The regression was simply that we've now started doing the correct layout inside of that incorrect intrinsic width.

Interesting things happen for different styles for .input-group-btn:

  Style                 current Gecko behavior           Chromium behavior

  display: table-cell   narrow intrinsic, narrow layout  wide intrinsic, wide layout
  display: table        narrow intrinsic, narrow layout  wide intrinsic, wide layout
  display: inline-table wide intrinsic, wide layout      wide intrinsic, wide layout
  overflow: hidden      narrow intrinsic, wide layout    wide intrinsic, wide layout

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To add to comment 4, Edge matches Chromium.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, if I force a narrow width (.input-group { width: 42px}) rather than using the intrinsic width, then:

 * for display: table-cell, display: table, and display:inline-table (on .input-group-btn), Edge, Gecko, and Chromium all do the narrow (wrapped) layout
 * for display:block;overflow:hidden (on .input-group-btn), Edge, Gecko, and Chromium all do the wide layout

So the only thing I see here that doesn't match other browsers is the intrinsic width calculation.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixing the intrinsic width calculation here requires changing the logic around nsIFrame::InlinePrefISizeData::ForceBreak.  In the current code we fully process the floats for every call to ForceBreak().  Fixing this presumably requires allowing floats to be next to multiple lines for the purposes of intrinsic width computation.  It requires testing to see if this means just not clearing mFloats in some cases, or requires buffering the maximum intrinsic widths of the multiple lines before processing mFloats.  I think the latter is probably more sensible, but they have different behaviors, and other engines may do one, the other, or something entirely different.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Jet, can we fix this for 48?

Comment 9

[Jet Villegas (inactive)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> Jet, can we fix this for 48?

The fixes described in comment 7 are changes we'd want to bake for a while, so I don't recommend going straight to Beta once we have patches.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #7)
> Fixing the intrinsic width calculation here requires changing the logic
> around nsIFrame::InlinePrefISizeData::ForceBreak.  In the current code we
> fully process the floats for every call to ForceBreak().  Fixing this
> presumably requires allowing floats to be next to multiple lines for the
> purposes of intrinsic width computation.  It requires testing to see if this
> means just not clearing mFloats in some cases, or requires buffering the
> maximum intrinsic widths of the multiple lines before processing mFloats.  I
> think the latter is probably more sensible, but they have different
> behaviors, and other engines may do one, the other, or something entirely
> different.

Although the latter does seem to make more sense, it doesn't match other engines behavior.

It seems to me what Blink and Edge do here is (in Gecko's term) they do not clear mFloats if the current line has nothing other than placeholders.

I guess it probably makes more sense if we do not clear mFloats when the current line is effectively empty (has mCurrentLine == 0), but that doesn't match other engines' behavior either. They do a force break even if there is only an additional empty span.

So the question is what behavior do we want here.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: proposed patch

This is the proposed patch. It is simple, and at least matches other engines' behavior for this case.

Comment 12

[Jet Villegas (inactive)]

I'm going to mark this 'fix-optional' for 48. I think this one should ride the trains for a bit.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8765367 [details] [diff] [review]
proposed patch

I don't think this is right.  I think it will break things like the intrinsic width of this case that doesn't involve floats at all:

  <div style="width: -moz-max-content; background: yellow">
    Hello
    <div>World</div>
  </div>

since data.mCurrentLine will be nonzero when we start to calculate the intrinsic width of the inner div.


What effect is it that you're trying to achieve?

(See also comment 7.)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Also, if we don't have a reftest for the case in comment 13, we should!  Did you see what happens to tests with the above patch?)

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #13)
> Comment on attachment 8765367 [details] [diff] [review]
> proposed patch
> 
> I don't think this is right.  I think it will break things like the
> intrinsic width of this case that doesn't involve floats at all:
> 
>   <div style="width: -moz-max-content; background: yellow">
>     Hello
>     <div>World</div>
>   </div>
> 
> since data.mCurrentLine will be nonzero when we start to calculate the
> intrinsic width of the inner div.

I don't understand what's wrong will happen to this case. When data.mCurrentLine is nonzero, we do a force break before the block as what we currently do, no?

> What effect is it that you're trying to achieve?

See comment 10.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, sorry, I was misreading.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> I guess it probably makes more sense if we do not clear mFloats when the
> current line is effectively empty (has mCurrentLine == 0), but that doesn't
> match other engines' behavior either. They do a force break even if there is
> only an additional empty span.

If Edge and Blink agree to this level of detail, we probably want to match them.

Do they both agree that if you have:

<div id="C">
  <block/>
  <inline/><float id="F"/>
  <block id="B"/>
</div>

or:

<div id="C">
  <block/>
  <float id="F"/><inline/>
  <block id="B"/>
</div>

then F's intrinsic width is not added to B's intrinsic width when computing C's intrinsic width, but if you remove the inline, then it is?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(and also worth testing the same cases with the first child <block/> removed)

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

For comment 16 and 17.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase for intrinsic width with empty span

(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #16)
> then F's intrinsic width is not added to B's intrinsic width when computing
> C's intrinsic width, but if you remove the inline, then it is?

As far as I test, yes.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

See the attached testcase for the behavior.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Attachment: proposed patch 2

This probably matches other engines' behavior better, but I think it's complicated, and doesn't make more sense. I'd still prefer the first one until we see new issues come.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8772728 [details] [diff] [review]
proposed patch 2

I prefer the second one over the first, since conditioning behavior on whether a value is currently 0 is very unusual for layout algorithms.  An emptiness test is more normal.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1260031 - Not force break before a block when calculating intrinsic width if the current line is empty and the block cannot intersect floats.

Review commit: https://reviewboard.mozilla.org/r/73294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73294/

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1260031 followup - Remove unused parameter of BlockReflowInput::ComputeBlockAvailSpace.

Review commit: https://reviewboard.mozilla.org/r/73296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73296/

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8783497 [details]
Bug 1260031 - Not force break before a block when calculating intrinsic width if the current line is empty and the block cannot intersect floats.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73294/diff/1-2/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8783498 [details]
Bug 1260031 followup - Remove unused parameter of BlockReflowInput::ComputeBlockAvailSpace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73296/diff/1-2/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Try looks good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=335cd4e63afe

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Likely wontfix for 49, as we are heading into late beta. It can probably ride the trains with 51.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8783497 [details]
Bug 1260031 - Not force break before a block when calculating intrinsic width if the current line is empty and the block cannot intersect floats.

https://reviewboard.mozilla.org/r/73294/#review73418

::: layout/generic/nsBlockFrame.cpp:790
(Diff revision 2)
>                 line->IsEmpty() ? ", empty" : "");
>        }
>        AutoNoisyIndenter lineindent(gNoisyIntrinsic);
>  #endif
>        if (line->IsBlock()) {
> +        if (!data.mEmptyLine || BlockCanIntersectFloats(line->mFirstChild)) {

Ah, interesting.  I didn't realize before this behavior was specific to things that establish new formatting contexts.  Does that match other engines?

::: layout/generic/nsIFrame.h:1840
(Diff revision 2)
> +    // True if the current line contains nothing other than placeholders.
> +    bool mEmptyLine;

Could you rename mEmptyLine to mLineIsEmpty?  I think that's more consistent with other code.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8783498 [details]
Bug 1260031 followup - Remove unused parameter of BlockReflowInput::ComputeBlockAvailSpace.

https://reviewboard.mozilla.org/r/73296/#review73422

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8783497 [details]
Bug 1260031 - Not force break before a block when calculating intrinsic width if the current line is empty and the block cannot intersect floats.

https://reviewboard.mozilla.org/r/73294/#review73418

> Ah, interesting.  I didn't realize before this behavior was specific to things that establish new formatting contexts.  Does that match other engines?

If the block can intersect floats, it would intersect floats, and consequently the block would start from the block start of the content box of its container, so we need a force new line for that.

As far as I can see, the behavior matches other engines when the block element has "display: block" at least.

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8783497 [details]
Bug 1260031 - Not force break before a block when calculating intrinsic width if the current line is empty and the block cannot intersect floats.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73294/diff/2-3/

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8783498 [details]
Bug 1260031 followup - Remove unused parameter of BlockReflowInput::ComputeBlockAvailSpace.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73296/diff/2-3/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f64102123512
Not force break before a block when calculating intrinsic width if the current line is empty and the block cannot intersect floats. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/e0c257e6ad66
followup - Remove unused parameter of BlockReflowInput::ComputeBlockAvailSpace. r=dbaron

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f64102123512
https://hg.mozilla.org/mozilla-central/rev/e0c257e6ad66

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Clear ni? for dbaron.

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

I guess we are not going to uplift this patch. It could potentially cause regression for some cases.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

This got backed out from 51 & 52 over in bug 1325496 for causing bug 1322843.