958249 - Border on :first-letter not work correctly from version 25

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[mnvk83]

Attachment: first-letter-bug.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.72 Safari/537.36

Steps to reproduce:

Use next CSS:
.some-class:first-letter {
  display: inline-block;
  margin-right: 5px;
  padding: 5px
  border: 2px solid #000;
}


Actual results:

Bottom border of the first letter isn't rendered


Expected results:

First letter have complete border

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Definitely a regression.  I'll take a look...

Comment 2

[Mats Palmgren (inactive)]

I haven't checked, but I suspect bug 836957 is the culprit.

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix+reftest

I guess just "return 0" works too (like the old code did) because the
style contexts for the continuations will always have the initial
value for border etc.  But it seems more correct to explicitly skip
all sides for the continuations.

https://tbpl.mozilla.org/?tree=Try&rev=0ef8ffb9f0a5

The suspected regression:
https://bugzilla.mozilla.org/attachment.cgi?id=708842&action=diff#a/layout/generic/nsFirstLetterFrame.cpp_sec2

Comment 4

[Daniel Holbert [:dholbert]]

Ah, so the problem is that nsFirstLetterFrame is inheriting the (nontrivial) GetSkipSides impl from nsSplittableFrame, rather than the (trivial) one from nsIFrame as I'd thought over in bug 836957.

Based on the end of bug 836957 comment 2, it sounds like there might be other things deriving from nsSplittableFrame that might suffer from this, too... :-/  I wonder if I just missed the fact that nsSplittableFrame had a GetSkipSides specialization?  Anyway, we/I can look into that in a separate bug; don't want to scope-creep this one.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #3)
> I guess just "return 0" works too (like the old code did) because the
> style contexts for the continuations will always have the initial
> value for border etc.  But it seems more correct to explicitly skip
> all sides for the continuations.

Can nsFirstLetterFrame even have continuations? (if so, what do they look like?)

I didn't think we allowed individual letters (or non-wrapped lines) of text to be split across pages.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I wonder if I just missed the fact that nsSplittableFrame had a
> GetSkipSides specialization?

Ah, I did not! Bug 836957 landed (for Firefox 21) in Feb 2013, *before* nsSplittableFrame had a GetSkipSides impl.

Then a few months later, bug 743402 added a GetSkipSides impl in nsSplittableFrame (for Firefox 25).

So bug 743402 is the most proximate thing that regressed this. (Which makes sense, since this bug's summary says this started happening in Firefox 25.)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> Can nsFirstLetterFrame even have continuations? (if so, what do they look
> like?)

Oh, it looks like its next continuation is just the remaining stretch of text. (the second letter and subsequent things on the line)

So any first-letter frame with stuff after it (Pa in the testcase) will currently be missing its bottom border, because it has a next continuation. Makes sense. So that's why we need to "return 0" instead of implicitly checking for GetNextContinuation (in the inherited impl).

I'm still not convinced it's worth checking GetPrevContinuation, though. Is it possible for a first-letter frame to have a prev continuation?

Comment 8

[Daniel Holbert [:dholbert]]

If it is possible for our frame to have a prev continuation here, could you include a test (or a chunk in the included test) that exercises that code-path, if it's not too much trouble?

Or alternately, if it's not possible (which I'm guessing is the case), let's drop that special case and just make this return 0. (Maybe even including an assertion that GetPrevContinuation() is null, to be sure it's not possible.)

Comment 9

[Mats Palmgren (inactive)]

Attachment: alternative version

(In reply to Daniel Holbert [:dholbert] from comment #8)
> could you include a test (or a chunk in the included test) that exercises that
> code-path, if it's not too much trouble?

I believe it's impossible.  Only the first continuation has the style context
associated with the ::first-letter style.  The remaining continuations have a
separate style context with initial values ('none') for border etc, so we don't
call GetSkipSides for those frames, at least not currently.

> Or alternately, if it's not possible (which I'm guessing is the case), let's
> drop that special case and just make this return 0. (Maybe even including an
> assertion that GetPrevContinuation() is null, to be sure it's not possible.)

Yeah, we can assert that GetSkipSides isn't called on later continuations,
but I slightly prefer the first version because from an API point of view
it should be possible to call nsIFrame::GetSkipSides on any frame (and get
the right result).

I don't feel strongly about it though - either patch is fine with me.
It's easy to to remove this restriction in the future if need be.

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8358177 [details] [diff] [review]
alternative version

># HG changeset patch
># Parent b395d82a82366f63ff62c217f6524db7af7b87e4
># User Mats Palmgren <[e-mail]>
>Bug 958249 - The first ::first-letter continuation should display all sides of the border, the following continuations none.  r=dholbert

(This needs an update to reflect what the alternate patch does. Probably best to just drop the mentions of continuations here.)

>+nsFirstLetterFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
>+{
>+  MOZ_ASSERT(!GetPrevContinuation(),
>+             "We only handle the first ::first-letter continuation b/c"
>+             " the remaining continuations are not expected to have"
>+             " properties that require calls to GetSkipSides");

This confuses me slightly. Any remaining continuations aren't going to be instances of nsFirstLetterFrame, are they?  IMHO, that's the important thing that makes this assert-worthy. (as opposed to special-case-for-correctness-that-we-don't-expect-to-visit, per your prev. patch)

A message more like...
  "nsFirstLetterFrame should only exist at the start of a continuation chain. How do we have a previous continuation?"
...makes more sense to me (assuming it's true. :) I think it is, but I might be missing something.)

>+  return 0;  // first continuation displays all sides

Per above, I'd just say "// display all sides" here. (I think mentioning "first continuation" makes it sound like we expect to have multiple nsFirstLetterFrame continuations, which IIUC we can't)

r=me with prose tweaked per above comments. (but do let me know if I'm misunderstanding things)

Comment 11

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> This confuses me slightly. Any remaining continuations aren't going to be
> instances of nsFirstLetterFrame, are they?

All the continuations are nsFirstLetterFrames.  That's actually a frame
tree invariant (with one minor exception in nsTextFrame /
nsContinuingTextFrame but at least they have a subclass relationship).

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8358055 [details] [diff] [review]
fix+reftest

Thanks for the clarification. (I was loosely aware of that invariant at a page-splitting level, but hadn't realized it applied at the level of letter frames, too. Handy!)

Given that, the first patch makes more sense. (sorry for prompting you to post the alternate)

>+int
>+nsFirstLetterFrame::GetSkipSides(const nsHTMLReflowState* aReflowState) const
>+{
>+  if (GetPrevContinuation()) {
>+    return 1 << NS_SIDE_LEFT |
>+           1 << NS_SIDE_RIGHT |
>+           1 << NS_SIDE_TOP |
>+           1 << NS_SIDE_BOTTOM;
>+  }

It'd be worth adding a brief comment to this clause saying that we don't expect to actually hit it (per comment 9). Might even be worth an NS_ERROR or NS_WARNING, but a comment also works.

r=me with that. Thanks!

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8358177 [details] [diff] [review]
alternative version

[Canceling r+ on alternate patch, since now I prefer the original one, per previous comment. :) ]

Comment 14

[Mats Palmgren (inactive)]

No problem!

I added an NS_ERROR for the continuations case which revealed that we
actually do get calls here!  (loading layout/generic/crashtests/478185-1.html
triggers it).  I will file a follow-up bug on that b/c I think it's a minor
performance problem to call GetSkipSides (virtual) without first checking if
the frame has a border (the call comes from nsIFrame::GetBorderRadii).

So I landed it with just a code comment for now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d18cb326989c

Comment 15

[Mats Palmgren (inactive)]

Oh, now I get it --  'border-radius' also shapes the corner when there is no border
at all, so there are valid calls to this method for later continuations!

I'll try to make a reftest that triggers that and adjust the comment accordingly...

Comment 16

[Mats Palmgren (inactive)]

Nah, border-radius has an initial value for the later continuations, so it's
what I first thought - an unnecessary call that we should try to get rid of...
Filed bug 958907 on that.

Comment 17

[Carsten Book [:Tomcat]]

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