Closed Bug 1377188 Opened 5 years ago Closed 5 years ago

Change the UA 'box-sizing' style to 'content-box' for <hr>

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

While reviewing bug 1356114 I noticed that other browsers (at least Edge and Chrome) appear to style hr elements with 'box-sizing: content-box' whereas we use 'box-sizing: border-box'.
It seems a good-first-bug for layout ICs. WDYT?
Status: NEW → ASSIGNED
Priority: -- → P3
Attached patch patchSplinter Review
Attachment #8890409 - Flags: review?(dholbert)
(In reply to Astley Chen [:astley] from comment #1)
> It seems a good-first-bug for layout ICs. WDYT?

Maybe, although they could practically just be pointed to the code to change, so I'm not sure there would be much learning value. At any rate, I'd like to fix this before we land anything for bug 1356114, so I'm just going to fix this.
Comment on attachment 8890409 [details] [diff] [review]
patch

Review of attachment 8890409 [details] [diff] [review]:
-----------------------------------------------------------------

Could you include a reftest? e.g. I think the following testcases should render the same:
 data:text/html,<hr style="height: 4px; background: yellow; border: 1px solid black">
 data:text/html,<div style="height: 4px; background: yellow; border: 1px solid black">

(the only difference being "hr" vs. "div")

In Chrome, those two render the same right now, and I presume they will in Firefox after your patch, too. (they don't match in Firefox right now, due to the box-sizing difference)

r=me assuming a reasonable testcase
Attachment #8890409 - Flags: review?(dholbert) → review+
I did a due-dilligence bit of searchfox.org archeology to check when this "border-box" styling was added to <hr>, too. It was back in 2003, as part of bug 38370.  There was no discussion of "box-sizing" on the bug, so I don't know precisely why -- but there is a mention of "inset" borders, so I'm guessing this "box-sizing:border-box" was part of making those inset-looking borders also "inset" from a layout calculations perspective, too.)

Here is the commit in the gecko-dev repo (imported from CVS):
  https://github.com/mozilla/gecko-dev/commit/a9e2dece768571828947e56bdbd23abd3d0b24f9

Anyway -- as long as this is improving webcompat (and it seems like it is), this seems like an obvious win.
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20fe3c8c0540
part 1 - Change the UA 'box-sizing' style to 'content-box' for <hr>. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/606245cf6527
part 2 - Add a reftest to check the default value of 'box-sizing' on <hr>. r=dholbert
Thanks for that digging. I followed the history back far enough to know that it predated the move from CVS to hg, but the extra background is good to have.
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b49898ac9063
part 3 - Mark WPT's the-hr-element-0/width.html test as passing on Mac. r=orange CLOSEDTREE
Duplicate of this bug: 1163906
You need to log in before you can comment on or make changes to this bug.