Closed Bug 1405568 Opened 2 years ago Closed 2 years ago

div height 100% is way too tall at biltema.fi whose <!DOCTYPE html PUBLIC "" ""> triggers quirks even though should be standards

Categories

(Core :: HTML: Parser, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: kim, Assigned: hsivonen)

References

()

Details

(Keywords: regression)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170928180207

Steps to reproduce:

Open up http://www.biltema.fi/ - a Nordic ecommerence website.


Actual results:

On top-right of the page the "Ostoslista" (Shopping list) <div> stretches way too tall because of height:100% set for class .button__text--icon-right.


Expected results:

In all other browsers and current stable Firefox 55 the <div> is the same height as the other divs on top-right.
Component: Untriaged → Layout
Product: Firefox → Core
Regression in FF57. Daniel: WDYT? Thx!
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dholbert)
Priority: -- → P3
mozregression results:
Last good revision: 06ffe35b662bf5d6981a44a13fecb84726aa2d9f
First bad revision: 8c6d1061135491843c8dcfa0337d14e974d8e82d
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=06ffe35b662bf5d6981a44a13fecb84726aa2d9f&tochange=8c6d1061135491843c8dcfa0337d14e974d8e82d


Hence, this was regression from bug 1375701. (That's surprising, since at first glance, it looks like that bug was intended to be an optimization & not affect behavior.  But I double-checked just to be sure, and indeed, this command shows the bad behavior (using cset from bug 1375701):
mozregression --repo autoland  --launch 8c6d1061135491843c8dcfa0337d14e974d8e82d -a "http://www.biltema.fi/fi/"

...and this command shows the expected behavior (using parent of bug 1375701's cset):
mozregression --repo autoland  --launch 06ffe35b662bf5d6981a44a13fecb84726aa2d9f -a "http://www.biltema.fi/fi/"

Perhaps there's some CSS class that we're mistakenly applying here.  hsivonen, do you know how/why this might have regressed?
Blocks: 1375701
Flags: needinfo?(dholbert) → needinfo?(hsivonen)
Keywords: regression
Requesting tracking, as an unintended behavior change (website layout breakage) caused by a commit that was intended to just be an optimization, AFAICT.  We should fix this for 57 if we can.
I wonder if the site is using innerHTML in a way that switches it from quirks mode (where percent heights are resolved more freely) to standards mode (where they're treated as "auto" in many cases, including in this case) -- and the bug is, we're not picking up on that change anymore...

If I view the site's source directly, I can see it starts out with:
> <!DOCTYPE html PUBLIC "" "">
which view-source highlights in red as a "quirky doctype".

If I save the site locally (as "Web page, complete", which serializes the final DOM to some extent), then it gets saved with
> <!DOCTYPE html>
...and that local copy does not produce the bug.  Though -- if I manually edit the saved copy to use the original doctype (i.e. if I add PUBLIC "" ""), then the bug does reproduce, in Firefox and Chrome.
(I don't immediately see where the JS changes the doctype to the standards-mode one, but clearly it must, because it ends up with <!DOCTYPE html> showing up in our devtools Inspector and also in the "save-as-complete" DOM.)

hsivonen, I'm hoping you can take this -- let me know if you can't though and I can try to dig in further.
Summary: div height 100% is way too tall → div height 100% is way too tall at biltema.fi (which dynamically changes its doctype from quirks-mode to standards-mode)
Bug 1375701 probably broke string comparisons such that <!DOCTYPE html PUBLIC "" ""> accidentally became quirky.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)
There is no dynamic change to doctype. <!DOCTYPE html PUBLIC "" ""> and <!DOCTYPE html> has the same DOM representation, so they are serialized to the latter.
Summary: div height 100% is way too tall at biltema.fi (which dynamically changes its doctype from quirks-mode to standards-mode) → div height 100% is way too tall at biltema.fi whose <!DOCTYPE html PUBLIC "" ""> triggers quirks even though should be standards
Attachment #8916527 - Flags: review?(bugs)
Comment on attachment 8916527 [details]
Bug 1405568 - Return false from nsHtml5String::LowerCaseStartsWithASCII when this string is shorter than the literal.

https://reviewboard.mozilla.org/r/187662/#review192664

Looks reasonable.
Attachment #8916527 - Flags: review?(bugs) → review+
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef2a97daf308
Return false from nsHtml5String::LowerCaseStartsWithASCII when this string is shorter than the literal. r=smaug
Thanks!  Please request 57 uplift once this has made it to central, too (assuming you think it's safe enough).  It'd be great to avoid shipping this as a regression in 57.
Component: Layout → HTML: Parser
Kiitos Kim, kun raportoit bugista!
https://hg.mozilla.org/mozilla-central/rev/ef2a97daf308
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Daniel Holbert [:dholbert] from comment #12)
> Thanks!  Please request 57 uplift once this has made it to central, too
> (assuming you think it's safe enough).  It'd be great to avoid shipping this
> as a regression in 57.
Flags: needinfo?(hsivonen)
Comment on attachment 8916527 [details]
Bug 1405568 - Return false from nsHtml5String::LowerCaseStartsWithASCII when this string is shorter than the literal.

Approval Request Comment
> [Feature/Bug causing the regression]:

Bug 1375701.

> [User impact if declined]:

Layout problems on Web sites. (Makes Quantum Style look bad even though this is actually a flaw in a Quantum Flow fix.)

> [Is this code covered by automated tests?]:

Yes.

> [Has the fix been verified in Nightly?]:

Yes.

> [Needs manual test from QE? If yes, steps to reproduce]: 

No.

> [List of other uplifts needed for the feature/fix]:

No other uplifts.

> [Is the change risky?]:

No.

> [Why is the change risky/not risky?]:

Not risky, because the fix is very easy to understand and changes an existing exit condition to return a different (correct this time) boolean by breaking the condition out of the loop condition into a separate "if".

> [String changes made/needed]:

None.
Flags: needinfo?(hsivonen)
Attachment #8916527 - Flags: approval-mozilla-beta?
(If the patch doesn't apply cleanly to 57, running the WPT manifest regeneration should fix things.)
Comment on attachment 8916527 [details]
Bug 1405568 - Return false from nsHtml5String::LowerCaseStartsWithASCII when this string is shorter than the literal.

Recent regression, Beta57+
Attachment #8916527 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Henri Sivonen (:hsivonen) from comment #16)
> > [Is this code covered by automated tests?]:
> 
> Yes.
> 
> > [Has the fix been verified in Nightly?]:
> 
> Yes.
> 
> > [Needs manual test from QE? If yes, steps to reproduce]: 
> 
> No.

Setting qe-verify- based on Henri's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.