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

RESOLVED FIXED in mozilla29

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mnvk83, Assigned: mats)

Tracking

(4 keywords)

26 Branch
mozilla29
dev-doc-needed, regression, site-compat, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox26 affected, firefox27 affected, firefox28 affected, firefox29 affected)

Details

(URL)

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 8357966 [details]
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
(Reporter)

Updated

5 years ago
(Assignee)

Comment 1

5 years ago
Created attachment 8358014 [details]
Testcase #1

Definitely a regression.  I'll take a look...
Assignee: nobody → matspal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 2

5 years ago
I haven't checked, but I suspect bug 836957 is the culprit.
Blocks: 836957
Component: Layout: View Rendering → Layout: Block and Inline
Keywords: regression, testcase
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 3

5 years ago
Created attachment 8358055 [details] [diff] [review]
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
Attachment #8358055 - Flags: review?(dholbert)
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
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.
(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.
(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.)
Blocks: 743402
(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?
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.)
Flags: needinfo?(matspal)
(Assignee)

Comment 9

5 years ago
Created attachment 8358177 [details] [diff] [review]
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.
Attachment #8358177 - Flags: review?(dholbert)
Flags: needinfo?(matspal)
Comment on attachment 8358177 [details] [diff] [review]
alternative version

># HG changeset patch
># Parent b395d82a82366f63ff62c217f6524db7af7b87e4
># User Mats Palmgren <matspal@gmail.com>
>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)
Attachment #8358177 - Flags: review?(dholbert) → review+
(Assignee)

Comment 11

5 years ago
(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 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!
Attachment #8358055 - Flags: review?(dholbert) → review+
Comment on attachment 8358177 [details] [diff] [review]
alternative version

[Canceling r+ on alternate patch, since now I prefer the original one, per previous comment. :) ]
Attachment #8358177 - Flags: review+
(Assignee)

Comment 14

5 years ago
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
Flags: in-testsuite+
(Assignee)

Comment 15

5 years ago
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...
(Assignee)

Comment 16

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/d18cb326989c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Keywords: dev-doc-needed, site-compat

Updated

4 years ago
Duplicate of this bug: 969034
You need to log in before you can comment on or make changes to this bug.