1322843 - bbc iplayer Web page does not display properly.

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect, P3)

Description

[iii_iii]

Attachment: Screenshot_2016-12-10_18-54-43.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49a2
Build ID: 20161209112840

Steps to reproduce:

Visit page:

http://www.bbc.co.uk/iplayer/episode/b084xk6m/planet-earth-ii-5-grasslands

Scroll down to the part which says "get recommendations tailored to you"



Actual results:

The text "change location" and "change language" is displayed over the top of the "get recommendations tailored to you"


Expected results:

The text "change location" and "change language"  should be below as it is in firefox and chromium.

Comment 1

[Rainer Bielefeld]

FF 53.0a1 (2016-12-10) (32-bit) also shows this problem

Comment 2

[Benjamin Smedberg]

roxana, do you have evidence this is plugin-related? It doesn't seem so to me, it seems like a layout issue.

In particular in Chrome, the "get recommendations" bit is to the *right* of the "More from this programme" section, while in Firefox it is on the next line. This is due to sizing of the following DOM structure:

<div class="tvip-clearfix stream-list" style="width: 5206px;">
  <div class="carousel more-episodes selected">...</div>
  <div class="carousel recommendations">
    <h2>...</h2>
    <ul>
      <li class="stream-item">...</li> x20 elements float-left
    </ul>
  </div>
</div>

In both Chrome and FF, the width of each <ul> is 205px. But in Chrome, the width of the <ul> is 4100px (205*20), while in Firefox the width of the <ul> is 4229.08 px. I can't see why yet.

I have an offline testcase I can upload, with some minimizations. Interestingly, if I remove the include of "beacon.js.download" from the page, Chrome reproduces the same symptoms as Firefox. That file is related to comScore, but it's minimized-enough that I can't figure out what it's doing.

I'm going to move this back to untriaged, but I think this is worth spending a little time diagnosing and constructing a more minimal testcase; we don't want to cause webcompat issues by accident, and we need to figure out whether this is JS, CSS/layout, or something else.

Comment 3

[Benjamin Smedberg]

Attachment: bug1322843-small-testcase.zip - somewhat minimized testcase

Comment 4

[roxleitan]

Attachment: latest Nightly bad.png

After some investigations I found that the issue is not reproducible on FF release 48.0, please see attached screenshot (Release 48.0 good) and it is reproducible on Nightly 53.0a1 (see screenshot latest Nightly bad). 

Using Mozregression the pushlog found is:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dbe4b47941c7b3d6298a0ead5e40dd828096c808&tochange=91a0bc34de8a35b22749f9081600ea3be17a9ada

Last good revision: dbe4b47941c7b3d6298a0ead5e40dd828096c808
First bad revision: 91a0bc34de8a35b22749f9081600ea3be17a9ada

Comment 5

[roxleitan]

Attachment: Release 48.0 good.png

Comment 6

[Benjamin Smedberg]

[Tracking Requested - why for this release]: potential webcompat regression

Bug 1260031 seems like the most likely suspect here. xidorn/dbaron?

Roxana that patch landed for 51, does this impact beta?

Comment 7

[roxleitan]

Yes, the bug is reproducible on Firefox Beta 51.0b7 (Build ID: 20161212025550)

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: simplified testcase

If comment 2 is the real issue here, this is a simplified testcase for it.

On release, there is nothing red, but on aurora, there is a red block besides the green block.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

I'll look into this.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

I've tested that the added tests all fail on current Nightly, and pass with this patch, and they all pass on Chrome.

Although I think the code added here should be reasonable, I'm not confident enough to uplift this patch to beta (unless dbaron thinks it's okay). If we don't want to ship this regression, I would suggest we backout bug 1260031 from beta, since that was a regression from longer time ago.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Oh, hmmm, we still have over a month before next merge day... I guess it is okay to uplift to beta, then.

Comment 13

[Julien Cristau [:jcristau]]

Tracking for 51/52 as new regression.

Comment 14

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> Although I think the code added here should be reasonable, I'm not confident
> enough to uplift this patch to beta (unless dbaron thinks it's okay). If we
> don't want to ship this regression, I would suggest we backout bug 1260031
> from beta, since that was a regression from longer time ago.

I would also prefer to back out bug 1260031 from beta rather than try to backport this to beta.

Comment 15

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

Comment on attachment 8818445 [details]
Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

https://reviewboard.mozilla.org/r/98500/#review100816

So I don't think this interacts with what nsIFrame::InlinePrefISizeData::ForceBreak() does in a reasonable way.  See comments below for more detail.

It would also be good to have a more detailed description of what it is you're *trying* to do in the commit message.


That said, given the contents of the if that your new code is in an else of, this is already a reasonably obscure case, since it only affects what happens when we have a block formatting context with 'clear' specified on it following a sequence of inlines that are empty (in terms of in flow content) and contain floats.

::: layout/generic/nsBlockFrame.cpp:857
(Diff revision 1)
> +          switch (breakType) {
> +            default:
> +            case StyleClear::None:
> +              break;
> +            case StyleClear::Both:
> +              data.mFloats.Clear();

I don't understand why clearing mFloats is the right thing to do here.  This means that the intrinsic sizes of the floats themselves are completely ignored.  (Normally they'd be considered by nsIFrame::InlinePrefISizeData::ForceBreak().)

Why do you want data.mFloats.Clear() here rather than data.ForceBreak()?

::: layout/generic/nsBlockFrame.cpp:874
(Diff revision 1)
> +                LogicalSide floatSide = ToLogicalSide(floatType, containerWM);
> +                if (floatSide != clearSide) {
> +                  floats.AppendElement(floatInfo);
> +                }
> +              }
> +              data.mFloats = Move(floats);

The same comment (above, about calling data.mFloats.Clear()) applies here as well, since here you're discarding *part* of data.mFloats.

This case, though, is more complicated, since nsIFrame::InlinePrefISizeData::ForceBreak() considers the clear property on floats, e.g., to handle a "float: left; clear: right".  This code will presumably do very strange things in such cases.

Perhaps you need to add something like the ability to invoke ForceBreak() but only partially process the floats?

(How do Chrome and Edge handle these interesting cases?)

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1322843 part 1 - Add Reverse method to nsTArray.

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

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1322843 part 3 - Add reftests for this bug.

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

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8818445 [details]
Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

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

Comment 19

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

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #14)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #11)
> > Although I think the code added here should be reasonable, I'm not confident
> > enough to uplift this patch to beta (unless dbaron thinks it's okay). If we
> > don't want to ship this regression, I would suggest we backout bug 1260031
> > from beta, since that was a regression from longer time ago.
> 
> I would also prefer to back out bug 1260031 from beta rather than try to
> backport this to beta.

Could you request approval for this?

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #19)
> Could you request approval for this?

Done in bug 1325496. Do you think I should also request approval for aurora?

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8821045 [details]
Bug 1322843 part 1 - Add Reverse method to nsTArray.

https://reviewboard.mozilla.org/r/100420/#review101312

::: xpcom/glue/nsTArray.h:2025
(Diff revision 1)
> +  // This method reverses the array in place.
> +  void Reverse()
> +  {
> +    const size_type len = Length();
> +    for (index_type i = 0, iend = len / 2; i < iend; ++i) {
> +      mozilla::Swap(Elements()[i], Elements()[len - i - 1]);

Please pull out the `Elements()` calls here to a common variable, and index off that.

Comment 22

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

Once this lands on m-c, let's either uplift this work to aurora (for 52) or do the backout in bug 1325496 for aurora.

Comment 23

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

Comment on attachment 8818445 [details]
Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

https://reviewboard.mozilla.org/r/98500/#review101950

While I think I'm OK with the code here, the commit message and comments in this patch leave a lot to be desired.

In particular, since there isn't a spec for this behavior, I'd expect the commit message (or possibly the comments, if appropriate, instead of the commit message) to explain the behavior being implemented in enough detail that somebody could *write* a spec from those comments.  And I did explicitly request a more detailed commit message in comment 15.

Given that this is a pretty core piece of Web technology that's underspecified, and related to real interop problems, I'm marking this review- for insufficient commit message and comments.  (Even without that, I'd normally expect a patch posted for review to have enough information to figure out what you're trying to do without having to read the whole patch.)

You should also file a spec issue against css-sizing to have this behavior specified.

::: layout/generic/nsFrame.cpp:4574
(Diff revision 2)
> +      StyleFloat clearFloatType =
> +        aBreakType == StyleClear::Left ? StyleFloat::Left : StyleFloat::Right;

Please assert here that aBreakType is either Left or Right.

::: layout/generic/nsFrame.cpp:4576
(Diff revision 2)
> -    mFloats.Clear();
> +      mFloats.Clear();
> +    } else {
> +      nsTArray<FloatInfo> newFloats;
> +      StyleFloat clearFloatType =
> +        aBreakType == StyleClear::Left ? StyleFloat::Left : StyleFloat::Right;
> +      // Iterate the array reversely so that we can stop when there is

"in reverse", not "reversely"

and "there are", not "there is"

::: layout/generic/nsFrame.cpp:4577
(Diff revision 2)
> +    } else {
> +      nsTArray<FloatInfo> newFloats;
> +      StyleFloat clearFloatType =
> +        aBreakType == StyleClear::Left ? StyleFloat::Left : StyleFloat::Right;
> +      // Iterate the array reversely so that we can stop when there is
> +      // no longer any floats we need to keep.

This should explain what's going on at a much higher level.  Why are you keeping some of the floats and not others?  (Though perhaps if you expand the comment below as I request, all you need to add here is "(see below)".)

::: layout/generic/nsFrame.cpp:4583
(Diff revision 2)
> +          // If a float in the non-keeping side clears the keeping side,
> +          // any float before it shouldn't affect the next line.

This comment should be much clearer.  "non-keeping" isn't good English, for a start, but it's also important to explain that what you mean by that is "the floats that aBreakType clears, and which we therefore don't keep in mFloats".

I'd rewrite this comment as:

This is a float on the side that aBreakType clears, which means we're not keeping it in mFloats.  However, if this float has a 'clear' value that clears floats on the opposite side (which could be 'both' or one of 'left'/'right'), then this float also means that we clear floats on the other side.  Therefore, we should break out of the loop, and stop considering earlier floats to be kept in mFloats.

::: layout/generic/nsFrame.cpp:4593
(Diff revision 2)
> +            break;
> +          }
> +        }
> +      }
> +      newFloats.Reverse();
> +      mFloats = Move(newFloats);

This needs a good explanation of why it's OK to keep floats in mFloats that we've already considered the widths of above.  I think it is OK, but it's a strange aspect of this code, so you should explain it.

::: layout/generic/nsIFrame.h:1927
(Diff revision 2)
> +
>      InlinePrefISizeData()
>        : mLineIsEmpty(true)
>      {}
>  
> -    void ForceBreak();
> +    void ForceBreak(StyleClear aBreakType = StyleClear::Both);

This should have a comment explaining what the parameter means.  (It's far from trivial!)

Comment 24

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

Which of the reftests failed without part 2?

(Note that if you structure the patch queue so that you add the tests in the earlier patch, and then change the expectations in the later one, I don't need to ask that.  Alternatively, you could include that information in the commit message.)

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

All four reftests fail without part 2 on Nightly. I'll include this in the commit message of part 3.

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821045 [details]
Bug 1322843 part 1 - Add Reverse method to nsTArray.

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

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8818445 [details]
Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

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

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821046 [details]
Bug 1322843 part 3 - Add reftests for this bug.

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

Comment 29

[Astley Chen (inactive)]

Given that in bug 1325496, the regression patch has been backed out from beta & aurora, no longer affected these 2 channel versions.
Thanks for Xidorn's prompt support.

Comment 30

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

Comment on attachment 8818445 [details]
Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

https://reviewboard.mozilla.org/r/98500/#review102058

::: layout/generic/nsFrame.cpp:4575
(Diff revision 3)
>      mCurrentLine = NSCoordSaturatingAdd(mCurrentLine, floats_done);
>  
> +    if (aBreakType == StyleClear::Both) {
> -    mFloats.Clear();
> +      mFloats.Clear();
> +    } else {
> +      // The force break not being asked to clear all floats means there

s/force break/break type/ ???

::: layout/generic/nsFrame.cpp:4576
(Diff revision 3)
>  
> +    if (aBreakType == StyleClear::Both) {
> -    mFloats.Clear();
> +      mFloats.Clear();
> +    } else {
> +      // The force break not being asked to clear all floats means there
> +      // may be some floats whose isize should contribute to intrinsic

contribute to *the* intrinsic isize

::: layout/generic/nsFrame.cpp:4577
(Diff revision 3)
> +    if (aBreakType == StyleClear::Both) {
> -    mFloats.Clear();
> +      mFloats.Clear();
> +    } else {
> +      // The force break not being asked to clear all floats means there
> +      // may be some floats whose isize should contribute to intrinsic
> +      // isize of the next line. The code below scans current mFloats

scans *the* current

::: layout/generic/nsFrame.cpp:4578
(Diff revision 3)
> -    mFloats.Clear();
> +      mFloats.Clear();
> +    } else {
> +      // The force break not being asked to clear all floats means there
> +      // may be some floats whose isize should contribute to intrinsic
> +      // isize of the next line. The code below scans current mFloats
> +      // and keep floats which are not cleared by this break. Note that

keep*s* floats

::: layout/generic/nsFrame.cpp:4596
(Diff revision 3)
> +          newFloats.AppendElement(floatInfo);
> +        } else {
> +          // This is a float on the side that this break directly clears
> +          // which means we're not keeping it in mFloats. However, if
> +          // this float clears floats on the opposite side (via a value
> +          // of either 'both' or one of 'left'/'right'), any remaining

maybe add "(earlier)" after "remaining"

::: layout/generic/nsIFrame.h:2030
(Diff revision 3)
> +     * Finish the current line and start a new line.
> +     *
> +     * @param aBreakType controls whether isize of floats would be
> +     * considered and what floats would be kept for the coming line:
> +     *  * |None| skips handling floats, which means no float would be
> +     *    cleared, and isize of floats are not considered either.
> +     *  * |Both| takes floats into consideration when computing isize
> +     *    of the current line, and clears all floats after that.
> +     *  * |Left| and |Right| do the same as |Both| except that they only
> +     *    clear floats on the given side (floats on the other side may
> +     *    be indirectly cleared by other floats, though).
> +     * All other values of StyleClear must be converted to the four
> +     * physical values above for this function.

So this text is confusing because it uses "clear" to mean both "empty a list"/"remove from a list" and "apply the 'clear' property".

You should use "remove" for removing from the list of floats to avoid that ambiguity.

also:
s/isize of floats would be considered/isizes of floats are considered/
s/what floats would be kept/which floats are kept/
s/coming line/next line/
s/no float would be cleared/no floats are removed/
s/isize of floats are/isizes of floats are/

and finally, replace:
clear floats on the given side (floats on the other side may
with:
remove floats on the given side, and any floats on the other side that are prior to a float on the given side that has a 'clear' property that clears them

Comment 31

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

Comment on attachment 8821046 [details]
Bug 1322843 part 3 - Add reftests for this bug.

https://reviewboard.mozilla.org/r/100422/#review102060

Could you also add a test or two covering the case where the BFC does *not* clear the float because the clear value is for the wrong side?

Comment 32

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8818445 [details]
Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

https://reviewboard.mozilla.org/r/98500/#review102058

> s/force break/break type/ ???

I mean "force break". Probably better "This force break". If using "break type", this sentence should probably be "Break type not clearing all floats means" or something, I guess.

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821045 [details]
Bug 1322843 part 1 - Add Reverse method to nsTArray.

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

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8818445 [details]
Bug 1322843 part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak.

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

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8821046 [details]
Bug 1322843 part 3 - Add reftests for this bug.

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e30a2cab8a6
part 1 - Add Reverse method to nsTArray. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ea2d213a90c6
part 2 - Conditionally keep some floats in InlinePrefISizeData::ForceBreak. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/016ea18d1fb0
part 3 - Add reftests for this bug. r=dbaron

Comment 37

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #32)
> I mean "force break". Probably better "This force break". If using "break
> type", this sentence should probably be "Break type not clearing all floats
> means" or something, I guess.

Do you mean "This call to ForceBreak" or "This forced break"?

"force" is a verb in this context, so it's not appropriate to put "this" in front of it.

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #37)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #32)
> > I mean "force break". Probably better "This force break". If using "break
> > type", this sentence should probably be "Break type not clearing all floats
> > means" or something, I guess.
> 
> Do you mean "This call to ForceBreak" or "This forced break"?

Oh, I mean "This forced break". Anyway, I've used a different expression. Hope that's fine.

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9e30a2cab8a6
https://hg.mozilla.org/mozilla-central/rev/ea2d213a90c6
https://hg.mozilla.org/mozilla-central/rev/016ea18d1fb0

Comment 40

[Ciprian Georgiu, Desktop QA]

I've managed to reproduce this issue on a Nightly build from 2016-12-10.

This is verified fixed on latest Nightly 53.0a1 (2017-01-03) on the following OSes:
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS
- Windows 10 x64