1320660 - inline-block element is clearing floating elements differently in nightly 53.0a1 (2016-11-27) (64-bit) for OS X

Status: RESOLVED WORKSFORME

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

Description

[julian]

Attachment: Screen Shot 2016-11-28 at 12.48.01.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161127030208

Steps to reproduce:

Browse https://adblockplus.org/


Actual results:

See video positioned below "Want to block ads on your smartphone or tablet?" notice.


Expected results:

Video should be positioned to the left of "Adblock Plus" section (right).

Applying `vertical-align: top;` to the video iframe seems to fix this issue. I'm guessing  that this is a bug on your end (too, at least) since this doesn't happen in other browsers (last 2 versions, Safari 6, IE 8).

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Web site layput is broken

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d2192fd7bf45180fce623528e0d81d93a0657b74&tochange=ae7543286030724b9aa495792c86085265e0f711

regressed by: Bug 1291110

Comment 2

[Alice0775 White]

Attachment: a reduced html

Comment 3

[Marcia Knous [:marcia]]

Tracking 53+ since this affects website layout.

Comment 4

[Alice0775 White]

Attachment: a reduced html

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Julian, thank you for reporting this issue.  Alice0775, the reduced html is helpful! Thank you.

First of all, I cannot reproduce this issue (either the original adblock site or the reduced html in comment 4) on my MBP retina display, but I could see the video being push down by using an external low resolution display, so I guess it's somewhat related to font and screen resolution.

Anyway, this is exact the issue I fixed in Bug 1291110. Let me explain.

Both <section id="main"> and <div id="message-wrapper"> are float: right. Since inline elements cannot intersect floats, it's expected that the <iframe id="video"> is pushed down if it is too tall to fit into the space to the left of <section id="main">. 

On the external display, the video element seems just 1px too tall to fit the space. I could fix this issue by either 
a) increase the height of <section id="main"> from 307px to 308px, or
b) have <iframe id="video"> go before <div id="message-wrapper">.

If I shrink the height of <section id="main"> to about 302px, I could see the same behavior on Chrome 54 and Safari 10.0.1. So I feel it's possible some of the users might experience the same issue on other browsers with different font or screen resolution. So it'll be the best if the adblock site could be fixed.

BTW, on the old Firefox (before bug 1291110 was fixed), the video element will intersect the float <div id="message-wrapper"> even if it's too tall, so this issue doesn't happen.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

NI for comment 5.

Comment 7

[julian]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #5)
> Julian, thank you for reporting this issue.  Alice0775, the reduced html is
> helpful! Thank you.
> 
> First of all, I cannot reproduce this issue (either the original adblock
> site or the reduced html in comment 4) on my MBP retina display, but I could
> see the video being push down by using an external low resolution display,
> so I guess it's somewhat related to font and screen resolution.

Acknowledged. 

I was able to reproduce your results using an MBP and an external display.

> 
> Anyway, this is exact the issue I fixed in Bug 1291110. Let me explain.
> 
> Both <section id="main"> and <div id="message-wrapper"> are float: right.
> Since inline elements cannot intersect floats, it's expected that the
> <iframe id="video"> is pushed down if it is too tall to fit into the space
> to the left of <section id="main">. 

Acknowledged.

Thank you for explaining.

> 
> On the external display, the video element seems just 1px too tall to fit
> the space. I could fix this issue by either 
> a) increase the height of <section id="main"> from 307px to 308px, or
> b) have <iframe id="video"> go before <div id="message-wrapper">.

Acknowledged.

Thank you for the suggestions.

> 
> If I shrink the height of <section id="main"> to about 302px, I could see
> the same behavior on Chrome 54 and Safari 10.0.1. So I feel it's possible
> some of the users might experience the same issue on other browsers with
> different font or screen resolution. So it'll be the best if the adblock
> site could be fixed.

Acknowledged.

Thank you for investigating this issue. And I agree with your findings.

> 
> BTW, on the old Firefox (before bug 1291110 was fixed), the video element
> will intersect the float <div id="message-wrapper"> even if it's too tall,
> so this issue doesn't happen.

Comment 8

[julian]

 > If I shrink the height of <section id="main"> to about 302px, I could see
> the same behavior on Chrome 54 and Safari 10.0.1. So I feel it's possible
> some of the users might experience the same issue on other browsers with
> different font or screen resolution. So it'll be the best if the adblock
> site could be fixed.

Shouldn't the fixed hight negate the effect of different font sizes / resolution here?

Both the main section and the iframe have fixed heights of 307px (if you take padding and border into consideration)?

Comment 9

[julian]

We've (AdblockPlus websites team) reduced this issue (or another related issue) to the following example:

https://jsfiddle.net/d8zp09af/6/

Where the iframe appears beside on most browsers and below on Firefox. This doesn't seem to reproduce the original issue exactly because the original issue only occurs on low-dpi screens.

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment 8:
> Shouldn't the fixed hight negate the effect of different font sizes /
> resolution here?
> 
> Both the main section and the iframe have fixed heights of 307px (if you
> take padding and border into consideration)?

I think it's the baseline of the different font that could affect the layout of the inline elements. The default value of the vertical-align property is baseline. That's why "vertical-align" in comment 0 could change the result.

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment 9:
> https://jsfiddle.net/d8zp09af/6/
> 
> Where the iframe appears beside on most browsers and below on Firefox. This
> doesn't seem to reproduce the original issue exactly because the original
> issue only occurs on low-dpi screens.

Thanks for the test case. If I changed <iframe> to <div>, then the iframe block will appear beside the main block. There must be something fishy that affect the calculation of height of the <iframe> in Firefox. Need to investigate further. At least it doesn't look like a regression of the float layout.

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: iframe-align.html

I turn the test case in comment 9 into a standalone html with some modification.

The vertical alignment of <iframe> is different from the <span> even if both of them have the same css class. There might be some historical reason behind it which is beyond my knowledge. (Note that Chrome and Safari behave similarly, but the <span> overlaps with the float.)

As for the the original example, the frame tree of the <iframe> is something like the following. The calculation of the height of <iframe> is correct. The <iframe> has height 6120 app unit, which is 102px. Due to the vertical alignment of the <iframe>, the height of the line is 6360 app unit, which is 106 px. That's why the line containing the float cannot fit into the space.

line 7f24105f6758: count=1 state=inline,clean,prevmarginclean,impacted,not wrapped,before:nobr,after:nobr[0x110] {0,12240,12120,6360} <
  FrameOuter(iframe src=)(5)@7f24105f5388 [view=7f240ff28c80] {0,12240,12120,6120} [state=000002c800002210] [content=7f240f9daef0] [sc=7f241023d700]
                >

I think this example show the dark corner of the float layout. I'm afraid I cannot fix this without breaking others. My recommendation in comment 5 to fix the site still holds.  

David, is there something we should investigate further? Otherwise, I think we should close this bug.

Comment 13

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

Attachment: testcase from comment 9

Comment 14

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

I agree that this appears to be correct behavior.

I think we should (a) try to encourage the site to be fixed (there are multiple possible fixes, including using vertical-align:top/bottom on the iframe or inline-block so that it doesn't have space for descenders below it) and (b) watch closely for any additional related problems, which might mean we need to back out the fix, at least temporarily.

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Julian, would AdblockPlus websites team consider fix the site?  Beside the fix I suggested in comment 5, David also suggests using `vertical-align` in comment 14, which is the same fix you observed in comment 0.

Comment 16

[julian]

> Julian, would AdblockPlus websites team consider fix the site?

Yes.

Comment 17

[julian]

Commit landed: https://hg.adblockplus.org/web.adblockplus.org/rev/f02197110014

I'm still getting email reminders about this from bugzilla. Please let me know if you need anything else from us. I'm currently unaware of anything outstanding.

Comment 18

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Julian, thank you for telling us the site is fixed! I think this case is solved. Close this bug per comment 17.

(As a reminder, you might get more email reminders from bugzilla until you clear the needinfo request. So I usually check the "clears the needinfo request" below when replying someone's needinfo message.)

Comment 19

[Julien Cristau [:jcristau]]

marking fix-optional for 52 as the known webcompat issue was fixed on the server side, I guess we'll see if this shows up elsewhere.

Comment 20

[Astley Chen (inactive)]

I don't think we need to keep tracking this issue from RM side. Do you agree ?

Comment 21

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Astley Chen [:astley] UTC+8 from comment #20)
> I don't think we need to keep tracking this issue from RM side. Do you agree?
Agree. Clear the tracking flag 53 since the issue was fixed on server side per comment 17.

Comment 22

[Marcia Knous [:marcia]]

Changing 53 status flag so it falls off the release health dashboard.