New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 687551 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Use other robhogan account instead.
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Regression



Sign in to add a comment

Chrome 56 font size behavior on content's top navigation menu

Reported by mza...@gmail.com, Feb 1 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.76 Safari/537.36

Example URL:
https://osuairport.org/

Steps to reproduce the problem:
1. Update from 55 to 56 stable (or any 56+ branch)
2. Load https://osuairport.org
3. Observe the top navigation menu font sizes changing dynamically

What is the expected behavior?
The fonts on the navigation bar should be the same size.  This is working properly from the same web content codebase here: https://engineering.osu.edu/ and here: https://osuairportfbo.org/

What went wrong?
The font sizes are dynamically decreasing or increasing for certain navigation menus on these sites.

Does it occur on multiple sites: Yes

Is it a problem with a plugin? N/A 

Did this work before? Yes 55

Does this work in other browsers? Yes

Chrome version: 56.0.2924.76  Channel: stable
OS Version: 10.0
Flash Version:

 

Comment 1 by mza...@gmail.com, Feb 1 2017

I should clarify, that "3. Observe the top navigation menu font sizes changing dynamically" means changing compared to version 55 and prior.

Comment 2 by mza...@gmail.com, Feb 1 2017

This appears to be an issue on Mac after 55->56 update as well

Comment 3 by bokan@chromium.org, Feb 1 2017

Components: -Blink Blink>CSS
Labels: Needs-Bisect
It's hard to tell what's going wrong here. I can most clearly see this by reloading the page. It looks like we apply hover style initially for some reason. I also suspect the font size change looks like the inline style getting applied. Ticking/Unticking the font-size under element.style in DevTools looks really similar to what's going on. I'm going to route this to the style team for a closer look.

A bisect would be very helpful here though.

Comment 4 by bokan@chromium.org, Feb 1 2017

Cc: bokan@chromium.org
Labels: Needs-Triage-M56 Prestable-56.0.2924.76
Cc: rbasuvula@chromium.org
Labels: -Needs-Bisect -Needs-Triage-M56 hasbisect-per-revision M-58 OS-Linux OS-Mac
Owner: robhogan@chromium.org
Status: Assigned (was: Unconfirmed)
Tested in chrome stable #56.0.2924.87 and canary #58.0.3000.0 on Win 7 able to reproduce the issue.
Below are the Bisect Details:

Bisect Info:
------------
Good Build: 56.0.2906.0 (Revison-428890)
Bad Build: 56.0.2907.0 (Revision-429169)

Bisect URL:
-----------
You are probably looking for a change made after 429108 (known good), but no later than 429109 (first known bad).
CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/bb98274e9983636f2b903b48779b75e54c24429a..b5243ab8c1a95305aff233a6175aa3a1f94d092f

From the CL above, assigning the issue to the concern owner

@ robhogan :Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url: https://codereview.chromium.org/2441373002
Note: Able to reporduce the issue on Mac 10.12.2 & Ubuntu 14.04.
Labels: Hotlist-Interop
The page is applying an inline font-size style to each item in the menu, presumably because it's detecting something in the size of the item's container that it responds to.

Can the original reporter shed some light on the logic used to decide whether to apply this inline-style?

Comment 9 by mza...@gmail.com, Feb 8 2017

@ robhogan :we're applying a dynamic font size to that menu bar within Drupal so that the menubar can adjust size relative to the hundreds of menu items many of our users customize using an open source jquery library called textfill found here: https://jquery-textfill.github.io/  In fact, you can see behavior on their site, note the "Type Something..." pre-populated text box demonstrates the library this does not behave the same on Chrome 56 vs 55 as well.
I get what appears to be the same behaviour on that page in both FF and Chrome. Maybe I'm missing a step?
CHrome-Screenshot from 2017-02-08 19-48-34.png
272 KB View Download
Firefox-Screenshot from 2017-02-08 19-48-54.png
255 KB View Download

Comment 11 by meade@chromium.org, Feb 13 2017

Labels: -Type-Bug Update-Weekly Type-Bug-Regression
Status: Unconfirmed (was: Assigned)
Components: -Blink>CSS Blink>Layout
Status: Untriaged (was: Unconfirmed)
Looking at https://osuairport.org in chrome 56 and FF 51.0.1 there is a noticable difference in the vertical alignment of the menu at the top of the page (element: 
<li id="menu-1241-1" class="sf-depth-1 menuparent title-about">...</li>)

If we remove the vertical-align:middle from this element then the text shifts under the menu in chrome and to the top of the menu in FF.

We were unable to observe the font size change described in the bug, but there is a noticeable difference in the page when loaded in FF to chrome.

Inspecting the element in each browser shows that they have the same css style, assigning to layout.
ff_bug_ss.png
918 KB View Download
ch_bug_ss.png
790 KB View Download

Comment 14 by mza...@gmail.com, Feb 20 2017

We recently implemented a workaround to our font size issue I will post the code we modified to accomplish that.
 
Cc: francois...@outlook.com
I think our rendering is correct. See discussion at https://bugs.chromium.org/p/chromium/issues/detail?id=637811

https://drafts.csswg.org/css-tables-3/#row-layout says:

"It is appropriate to resolve percentage heights on direct children of table cells if the cell is considered to have its height specified explicitly or the element is absolutely positioned, see http://www.w3.org/TR/CSS2/visudet.html#the-height-property."

Reduction attached.

Also cc'ing Francois, the spec editor, in case I've got it wrong here.

687551-2.html
338 bytes View Download
I didn't look at the issue but the test case looks wrong to me. 

Indeed, the <td> element itself has specified style "height:100%" and since its the height of the table itself is auto, we are in the case where "the height of the containing block is not specified explicitly (i.e., it depends on content height), and this element is not absolutely positioned, the value computes to 'auto'", so the <td> has a computed style of "height:auto" instead.

Since the <td> has "height:auto", we do not end up in the case where "the cell is considered to have its height specified explicitly" and the div ends up with "height:auto" as well following the same logic.

If the <table> element had "height:0" or something similar specified, then the test case would be right.
francois@: That part of the CSS2 spec has changed. It now reads: "If the height of the containing block is not specified explicitly (i.e., it depends on content height), and this element is not absolutely positioned, *the used height is calculated as if 'auto' was specified*." (https://drafts.csswg.org/css2/visudet.html#the-height-property)

So interacting with this from https://drafts.csswg.org/css-tables-3/#computing-the-table-height I think that the cell still does have a computed height that is a percentage:

"Once the final size of the table and the rows has been determined, the content of the table-cells must also go through a second layout pass, where, if appropriate, percentage-based heights are this time resolved against their parent cell used height.

It is appropriate to resolve percentage heights on direct children of table cells if the cell is considered to have its height specified explicitly or the element is absolutely positioned, see CSS 2.

It is further clarified that a cell is considered to have its height specified explicitly if the computed height of the cell or any of its table ancestors is a length or percentage."

Also, (though I don't think it's relevant in this bug yet, depends where we go), it's not clear from any spec yet what the containing block of a table cell is. Do you have any sense of the right answer on that one?

(And sorry, that's partly my fault for linking to the old spec in #16. I need to remove it from my bookmarks).

Comment 20 by e...@chromium.org, Feb 21 2017

Labels: Needs-Feedback
The fact this errata has implication on other specs is a bug per se. It was not intended for this errata to have any impact on layout, but you are right it does due to how a lot of spec text is written all over the place.

I filed an issue to track this down. 
https://github.com/w3c/csswg-drafts/issues/1051

In the meantime, I would advise to fix the Chrome regression.
HI Francois, looks like this was discussed on the css-wg call. I'm not clear on the implications of your note though - do I need to ahead and get Blink to treat the computed height as auto?
Tested this issue Ubuntu (#58.0.3026.3), Mac(#58.0.3027.3) and Windows (#58.0.3027.0) and observed fonts on the navigation bar is same size

Attaching the screenshot for reference.

mzazon@ could you please look into it and let us know your observations
Issue 687551.png
197 KB View Download

Comment 24 by mza...@gmail.com, Mar 2 2017

We had to implement a workaround in javascript to force a small,static font size in our menus and the airport site got that update recently.

This site has not cleared cache yet and is a good test (for now) https://honda.osu.edu/
Able to reproduce the issue on Win-10 using latest canary #59.0.3046.0.

The issue is not seen in firefox.

Attaching screen shots of both chrome canary and firefox.

robhogan@ - Could you please have a look into this issue.

Thanks...!!
canary.JPG
129 KB View Download
francois@ - last question I hope.
Am I right in thinking the table child of the cell in the second test should also have a zero height? Firefox and Chrome currently special-case the treatment of tables there and grow the table to fill the cell, but I don't see why.

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/LayoutTests/fast/table/percent-height-content-in-percent-height-cell-with-height-from-row.html


francois@ - I meant to say the 'first' test in my previous comment, not the second.


I definitely do agree with you that being inconsistent between a block and a table doesn't make any sense. On that we are 100% clear.

The spec currently states that both should act like your test does assert it should, but that was not the initial intention and is only due to the css 2.1 errata. 

Thinking about this further, though, I think I would agrue we should keep the text as it is now; the css 2.1 behavior is the weird one here, not the table behavior. The reason why I say that is that percentage-heights always have an effect on the table layout whether or not the table has a specified height, so they don't behave as auto like in the display:block;height:100% case that css 2.1 covered.

As a result, yep, I do agree with your test case, but that will need a working group resolution. I can't just change that on my own. If what your test case asserts is your current behavior after your fixes, I wouldn't change it, I do not expect the working group to object to this change. 
Chrome currently passes https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/LayoutTests/fast/table/percent-height-content-in-percent-height-cell-with-height-from-row.html, if I was to fix the bug here then it would fail both parts of the test (and match Firefox by doing so) - the #test table and #test div would both get an auto height of zero.

It sounds like I should just do nothing and that the spec will change to match Chrome's current rendering of the test case and https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=272016? Is that correct?


Hum, it seems we already resolved on that and decided to keep the original meaning, in https://github.com/w3c/csswg-drafts/issues/1051. So, let's rephrase taking this into account.

I definitely do agree with you that being inconsistent between a block and a table doesn't make any sense. On that we are 100% clear.

The current intention of the spec is to have both be zero-pixel tall. The current word of the spec is to have both grow-to-fit. That was due to my ignorance of the css 2.1 errata.

If we want to change the behavior from min-content-height to grow-to-fit, we would need to actually revert the decision we made before in https://github.com/w3c/csswg-drafts/issues/1051. There could be ground for doing so since percentages work in a very different way for tables, though. Not sure that is really worth it, we should probably just go along with that decision.

To sum up, since the group already resolved on updating the text of specs that assumed the old css 2 behavior, both should use min-content-height indeed and not have their layout recomputed when the height of the cell is ultimately computed. It would be the case if the table had a specified height though.
"To sum up, since the group already resolved on updating the text of specs that assumed the old css 2 behavior, both should use min-content-height indeed and not have their layout recomputed when the height of the cell is ultimately computed. It would be the case if the table had a specified height though."

Hi Francois - OK, and a table with a percent height but where the height of its containing block is not specified explicitly (i.e. it depends on content height, or in other words has auto or percent all the way up the containing block chain) doesn't have a 'specified height' is that correct?
Correct. As soon as you are out of the table, CSS 2.1 applies normally and will discard the percentage value if the parent containing block of the table-root is auto-sized. This seems to happen in Firefox but not in Edge, I'll file a bug on that.
btw - you need to scroll down to see the new result, the old result is at the top.
I had a look by using the side-by-side diff on the text-dump files, and it looks alright to me.

It is somewhat funny to learn the percentage-height-of-table-in-tablecell exception you discovered seems to come from an old IE bug. It has obviously been fixed at the Microsoft side by now but, I guess, had been backported to other browsers by the time it got fixed...
Project Member

Comment 36 by bugdroid1@chromium.org, Apr 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15

commit 019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15
Author: robhogan <robhogan@gmail.com>
Date: Fri Apr 21 08:03:39 2017

Treat cells with percent height as auto if their container depends on content height

After some clarification with the css-tables spec author and the CSSWG, only
treat cells as having a height that percent values can resolve against if the cell
has an explicitly specified height. For example, if the cell has a percent height
but nothing in its container chain to resolve that percentage against it doesn't
have an explicitly specified height.

BUG= 687551 

Review-Url: https://codereview.chromium.org/2816313002
Cr-Commit-Position: refs/heads/master@{#466287}

[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/fast/table/percent-height-content-in-percent-height-cell-with-height-from-row-expected.txt
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/fast/table/percent-height-content-in-percent-height-cell-with-height-from-row.html
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/linux/tables/mozilla_expected_failures/bugs/bug131020-3-expected.png
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/linux/tables/mozilla_expected_failures/bugs/bug131020-3-expected.txt
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/linux/tables/mozilla_expected_failures/bugs/bug32205-4-expected.png
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/linux/tables/mozilla_expected_failures/bugs/bug32205-4-expected.txt
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/mac-mac10.9/tables/mozilla_expected_failures/bugs/bug131020-3-expected.png
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/mac/tables/mozilla_expected_failures/bugs/bug131020-3-expected.png
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/mac/tables/mozilla_expected_failures/bugs/bug32205-4-expected.png
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/win/tables/mozilla_expected_failures/bugs/bug131020-3-expected.png
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/LayoutTests/platform/win/tables/mozilla_expected_failures/bugs/bug32205-4-expected.png
[modify] https://crrev.com/019c92b5d6c58f4c240ea2b64bb748c9bb3a6c15/third_party/WebKit/Source/core/layout/LayoutTableSection.cpp

Status: Fixed (was: Untriaged)

Sign in to add a comment