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

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: arni2033, Assigned: xidorn)

Tracking

({regression})

42 Branch
mozilla53
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 fixed)

Details

(Whiteboard: [parity-IE][parity-Chrome][parity-Edge])

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
>>>   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)

Comment 1

3 years ago
Created attachment 8735276 [details]
reduced html

Updated

3 years ago
Whiteboard: [parity-IE][parity-Chrome][parity-Edge]
Created attachment 8742512 [details]
slightly simpler testcase, with colors
Created attachment 8742517 [details]
slightly simpler testcase, with colors
Attachment #8742512 - Attachment is obsolete: true
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)
(Assignee)

Updated

2 years ago
Flags: needinfo?(bugzilla)
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

2 years ago
Created attachment 8765367 [details] [diff] [review]
proposed patch

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)
status-firefox47: --- → wontfix
status-firefox49: --- → affected
status-firefox50: --- → affected
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.
status-firefox48: affected → fix-optional
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?)
(Assignee)

Comment 15

2 years ago
(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)
(Assignee)

Comment 18

2 years ago
For comment 16 and 17.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 19

2 years ago
Created attachment 8772721 [details]
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.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 20

2 years ago
See the attached testcase for the behavior.
(Assignee)

Comment 21

2 years ago
Created attachment 8772728 [details] [diff] [review]
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.
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)

Updated

2 years ago
Assignee: nobody → xidorn+moz
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8765367 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8772728 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Likely wontfix for 49, as we are heading into late beta. It can probably ride the trains with 51.
status-firefox49: affected → wontfix
status-firefox51: --- → affected
status-firefox50: affected → fix-optional
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+
(Assignee)

Comment 31

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

2 years ago
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

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f64102123512
https://hg.mozilla.org/mozilla-central/rev/e0c257e6ad66
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 36

2 years ago
Clear ni? for dbaron.
Flags: needinfo?(dbaron)
(Assignee)

Comment 37

2 years ago
I guess we are not going to uplift this patch. It could potentially cause regression for some cases.
status-firefox48: fix-optional → wontfix
(Assignee)

Updated

2 years ago
Depends on: 1325496
This got backed out from 51 & 52 over in bug 1325496 for causing bug 1322843.
status-firefox50: fix-optional → wontfix
status-firefox51: fixed → wontfix
status-firefox52: --- → wontfix
status-firefox53: --- → fixed
Target Milestone: mozilla51 → mozilla53
You need to log in before you can comment on or make changes to this bug.