421436 - Remove br 1-appunit width hack

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BR frames make themselves 1 unit wide in standards mode or when they're at the start of a line:

    if ( ll->CanPlaceFloatNow() ||
         aPresContext->CompatibilityMode() == eCompatibility_FullStandards ) {
...
      // XXX temporary until I figure out a better solution; see the
      // code in nsLineLayout::VerticalAlignFrames that zaps minY/maxY
      // if the width is zero.
      // XXX This also fixes bug 10036!
      // Warning: nsTextControlFrame::CalculateSizeStandard depends on
      // the following line, see bug 228752.
      aMetrics.width = 1;

This hack was introduced because nsLineLayout used to treat zero-width lines as completely empty and collapse them away. nsLineLayout no longer does that, so this hack can be removed.

Bug 10036 should not regress, because table cells containing a <br> will not have zero height. (Although the table cell emptiness check should be rewritten to not check the height, that is an unreliable approach.)

The code added to nsTextControlFrame::CalculateSizeStandard in bug 228752 was to work around this hack, so it can be removed.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note, this hack can actually cause problems by making spans a tiny bit wider than they should be.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Remove the hack. Adds tests for bug 10036 to prove we don't regress it. Also adds a test we fail on trunk because of the hack.

Comment 3

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

Comment on attachment 324915 [details] [diff] [review]
fix

r+sr=dbaron, if you also add tests equating:

<div>A<br>B</div> == <div>A</div><div>B</div>

<div>A<br><br>B</div> == <div>A</div><div><span style="visibility:hidden">X</a></div><div>B</div>

(probably for both quirks and standards modes)

getBoundingClientRect returns fractional pixels, right?  If not, you'd need to do the test for this bug inside a loop, adjusting the position across various subpixel positions.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, getBoundingClientRect returns fractional pixels. That's why the test fails on trunk.

I'll add those other tests. Thanks.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out to fix bug 439462. The tests are still in the tree, with one todo for the test that fails on trunk.

Comment 7

[Simon Montagu :smontagu]

So I tried to revisit this when I came across the code as something I would have to touch in bug 789096. Try run is at https://tbpl.mozilla.org/?tree=Try&rev=49732de28a1d

The UNEXPECTED-PASS errors in M4 are easily removed; more worrying are the reftest failures. They are all the same error: a difference of 12-15 (depending on platform) in the corners of the textarea border. The tests are already marked fuzzy(11,4), so one could just turn up the fuzziness to 15 and everything would pass. I'm not sure if that's such a great idea, though.

Bug 439462 doesn't seem to regress any more with the patch, and bug 465574 is fixed, so I should add testcases for both of those.

Comment 8

[Simon Montagu :smontagu]

Attachment: Patch

Try run with bug 1012220 and a test for bug 465574: https://tbpl.mozilla.org/?tree=Try&rev=d4ffa011bf0f.

I haven't been able to make a test for bug 439462, because I can't reproduce the regression in any source tree as far back as I can still build the source on my system.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8424451 [details] [diff] [review]
Patch

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

Excellent!

Comment 10

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aaa18232af4

Comment 11

[Carsten Book [:Tomcat]]

sorry had to backout this change since it caused on android test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=39932940&tree=Mozilla-Inbound

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we can just fuzz our way to victory here :-)

Comment 13

[Simon Montagu :smontagu]

Yeah, but I would like to understand what causes the single pixel differences. Maybe there is still some code somewhere that was introduced to compensate for the extra 1 app unit and I didn't remove?

Comment 14

[Simon Montagu :smontagu]

That said, I just saw that the failing test already has fuzzy-if(OSX==10.8,1,1)! I'll just extend that to Android and B2G

Comment 15

[Simon Montagu :smontagu]

Attachment: Patch for recheckin with test fixes

Comment 16

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f1ab75ea51

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3f1ab75ea51

Comment 18

[Daniel Holbert [:dholbert]]

This most recent landing regressed bug 439462 (as described in now-duplicate bug 1014807). So, we should back this out for now (smontagu agrees per bug 1014807 comment 5).  The tree's closed at the moment, so we can't backout yet -- I'm intending to back out when it reopens, but if I don't get to it, anyone else can feel free to do so.

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Simon Montagu :smontagu from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f1ab75ea51

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d6f9f692aa

Comment 20

[Daniel Holbert [:dholbert]]

Backout merged to m-c this morning: https://hg.mozilla.org/mozilla-central/rev/f2d6f9f692aa

Comment 21

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

(In reply to Daniel Holbert [:dholbert] from comment #18)
> This most recent landing regressed bug 439462 (as described in now-duplicate
> bug 1014807). So, we should back this out for now (smontagu agrees per bug
> 1014807 comment 5).  The tree's closed at the moment, so we can't backout
> yet -- I'm intending to back out when it reopens, but if I don't get to it,
> anyone else can feel free to do so.

This sounds similar to bug 465574 comment 6 -- which pointed to bug 439567 -- which I'm actually not sure is fixed.  Does fixing that allow this to make progress?

Comment 24

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

Note that an attempt to reland this also needs to remove the code added in bug 1242781, i.e., https://hg.mozilla.org/mozilla-central/rev/9d85d87c1179 .

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 26

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 421436 - Remove the hack that forces all <br> frames to be 1 app unit wide.

This patch is based on the previous attempt [1] in this bug, and removes the
code added in Bug 1242781.

The test_bug1216483.html change is worth an explanation. Each sub-test verifies
the blinking cursor (caret) position after pressing down arrow key in an
contenteditable container. The expected position is to the right of the letter
"a". Before this patch, the (anchorNode, anchorOffset) was
("element with '.second' style", 1). After this patch, it becomes
("a", 1), which is a valid representation of the caret position and
matches Google Chrome's behavior.

[1] https://hg.mozilla.org/mozilla-central/rev/c3f1ab75ea51

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b58abcb2d3d7
Remove the hack that forces all <br> frames to be 1 app unit wide. r=emilio

Comment 28

[agoloman]

Backed out for causing multiple failures.

• Backout link
• Push with failures
• Failure Log reftest failures
• Failure Log dt failures

Comment 29

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 421436 - Fix y coordinate when calling focusEditableField in browser_rules_grid-template-areas.js.

After apply the previous patch D238162, the return value of
getBoundingClientRect() can be different since the <br> do not have any
width.

This patch does not change any test expectation. It only fixed the y coordinate
passing to focusEditableField so that the call can be successful on the
grid-template-area value span, stored in gridRuleProperty.

Comment 30

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Add a separate patch to fix the dt failures for a devtools test.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d7a534b3e86
Remove the hack that forces all <br> frames to be 1 app unit wide. r=emilio
https://hg.mozilla.org/integration/autoland/rev/202dc9cb8c22
Fix y coordinate when calling focusEditableField in browser_rules_grid-template-areas.js. r=devtools-reviewers,nchevobbe

Comment 32

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest failure on test_accessiblecaret_cursor_mode.py

Backout link: https://hg.mozilla.org/integration/autoland/rev/65d10546b02a910eb543cd43c0cc5629e8dd2362
Log link: https://treeherder.mozilla.org/logviewer?job_id=495361266&repo=autoland&lineNumber=8995

Comment 33

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

The patch causes a regression in AccessibleCaret. Luckily, test_accessiblecaret_cursor_mode.py detected it. Filed bug 1948819 to investigate.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6448e1fd65ee
Remove the hack that forces all <br> frames to be 1 app unit wide. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a7b8caf56dfd
Fix y coordinate when calling focusEditableField in browser_rules_grid-template-areas.js. r=devtools-reviewers,nchevobbe

Comment 35

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/6448e1fd65ee
https://hg.mozilla.org/mozilla-central/rev/a7b8caf56dfd