Closed Bug 1260031 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout: Floats, defect)

42 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: arni2033, Assigned: xidorn)

References

Details

(Keywords: regression, Whiteboard: [parity-IE][parity-Chrome][parity-Edge])

Attachments

(5 files, 3 obsolete files)

>>>   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
Flags: needinfo?(dbaron)
Attached file reduced html
Whiteboard: [parity-IE][parity-Chrome][parity-Edge]
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
To add to comment 4, Edge matches Chromium.
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.
Summary: Search button is pushed down (displays on the 2nd line) on http://thirdworld.nl/ → [float pref width] Search button is pushed down (displays on the 2nd line) on http://thirdworld.nl/
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.
Jet, can we fix this for 48?
Flags: needinfo?(bugs)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(bugs)
Flags: needinfo?(bugzilla)
(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.
Attached patch proposed patch (obsolete) — Splinter Review
This is the proposed patch. It is simple, and at least matches other engines' behavior for this case.
Flags: needinfo?(bugzilla)
Attachment #8765367 - Flags: feedback?(dbaron)
Version: Trunk → 42 Branch
I'm going to mark this 'fix-optional' for 48. I think this one should ride the trains for a bit.
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.)
Attachment #8765367 - Flags: feedback?(dbaron) → feedback-
(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?)
(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.
Attachment #8765367 - Flags: feedback- → feedback?(dbaron)
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?
(and also worth testing the same cases with the first child <block/> removed)
For comment 16 and 17.
Flags: needinfo?(xidorn+moz)
(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.
Flags: needinfo?(xidorn+moz)
See the attached testcase for the behavior.
Attached patch proposed patch 2 (obsolete) — Splinter Review
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.
Attachment #8772728 - Flags: feedback?(dbaron)
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.
Attachment #8772728 - Flags: feedback?(dbaron) → feedback+
Assignee: nobody → xidorn+moz
Attachment #8765367 - Attachment is obsolete: true
Attachment #8772728 - Attachment is obsolete: true
Likely wontfix for 49, as we are heading into late beta. It can probably ride the trains with 51.
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.
Attachment #8783497 - Flags: review?(dbaron) → review+
Comment on attachment 8783498 [details]
Bug 1260031 followup - Remove unused parameter of BlockReflowInput::ComputeBlockAvailSpace.

https://reviewboard.mozilla.org/r/73296/#review73422
Attachment #8783498 - Flags: review?(dbaron) → review+
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.
Pushed by xquan@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f64102123512
https://hg.mozilla.org/mozilla-central/rev/e0c257e6ad66
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Clear ni? for dbaron.
Flags: needinfo?(dbaron)
I guess we are not going to uplift this patch. It could potentially cause regression for some cases.
Depends on: 1325496
Target Milestone: mozilla51 → mozilla53
You need to log in before you can comment on or make changes to this bug.