45091 - ::first-letter and openquote ::before should be together

Status: NEW

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

Description

[Erik van der Poel]

According to the CSS2 spec, :first-letter applies to any leading punctuation
character as well as the first normal letter. The following test case used to
cause a crash, but that was fixed. Now we need to make sure it works according
to spec:

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=10091

Comment 1

[Frank Tang]

remove Future for now.

Comment 2

[Frank Tang]

mark as moz1.0 for now.

Comment 3

[Frank Tang]

erik resign. reassign all his bug to ftang for now.

Comment 4

[Frank Tang]

move to future for now.

Comment 5

[basic]

so the :first-letter in this case should apply to the :before element?

If that is what it is suppost to be then it WFM in build 2001100303 win32

Comment 6

[basic]

ok I got it wrong. The bug still exist as the testcase needs to have
:first-letter apply to the *S* too. So I assume the problem is that the
open-quote is not reconized as a quote?

Comment 7

[Eli Friedman]

For anyone who looks: the problem is that, although :first-letter works properly
for normal punctuation, it does not work for punctuation inserted using :before.
 I _think_ that the problem is that the :before node is separate from the text
(there is a bug about :first-letter and image alt text, which I can't find at
the moment, that I think is related).

Comment 8

[Mats Palmgren (inactive)]

I've started looking at this a bit.  I'm unsure if what I'm doing is correct
though.  For exmaple, what should the frame tree look like for this case:
<p><span>"</span><span>T</span>ext</p>

CSS2.1 explicitly says that :first-letter is "innermost" which would mean:
FL = p:first-letter
<p><span><FL>"</FL></span><span><FL>T</FL></span>ext</p>

Is this correct?  then what about margins, borders and such?

Comment 9

[Anne (:annevk)]

Isn't...

<p><::before><::first-letter>"</::first-letter></::before><::first-letter>T</::first-letter>ext</p>

...more correct?

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (float)

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

I didn't know where to put the "IsPunctuationMark" functionallity... it's
now copy-pasted in "nsCSSFrameConstructor.cpp" from "nsTextFrame.cpp".
Maybe this should live in a string/i18n lib somewhere... please advice.

This patch implements multi frame :first-letters to the level of CSS 2.0,
i.e. it does not traverse child blocks. I have a patch for that too but the
layout part isn't working yet so I would like to do that as a second stage.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -uw)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Shared things can also go in nsLayoutUtils...

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

OK, I added the "IsPunctuationMark" stuff to nsLayoutUtils.

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -uw)

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm not likely to get to this review until after I get back to town...  So not likely to happen 
before the alpha milestone ends.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169014 [details] [diff] [review]
Patch rev. 2

>Index: layout/generic/nsIFrame.h

>+// Text reflow bit
>+#define TEXT_IN_WORD                                  0x04000000
>+
>+// First-letter layout bits
>+#define NS_FIRST_LETTER_FIRST_FRAME                   0x20000000
>+#define NS_FIRST_LETTER_LAST_FRAME                    0x40000000

Document that these only have to be here because the relevant concrete classes
don't have header files that you could put them in?  Also document which exact
concrete classes would use those bits.	And while you're here, document what
the bits mean?

>Index: layout/generic/nsBlockFrame.cpp

>+  if (nsLayoutAtoms::letterFrame == floatFrame->GetType()) {
>+    nsFrameState frameState = floatFrame->GetStateBits();
>+    if (!(frameState & NS_FIRST_LETTER_FIRST_FRAME)) {
>+      aFloatCache->mMargins.left = 0;
>+    }
>+    if (!(frameState & NS_FIRST_LETTER_LAST_FRAME)) {
>+      aFloatCache->mMargins.right = 0;
>+    }
>+  }

This could use a comment explaining why this is being done (though perhaps
documenting those flags will be sufficient?)

>Index: layout/generic/nsFirstLetterFrame.cpp
>+nsFirstLetterFrame::PaintSelf(nsPresContext*       
>+  if (!(frameState & NS_FIRST_LETTER_FIRST_FRAME)) {
>+    aSkipSides |= (1 << NS_SIDE_LEFT);
>+  }
>+  if (!(frameState & NS_FIRST_LETTER_LAST_FRAME)) {
>+    aSkipSides |= (1 << NS_SIDE_RIGHT);
>+  }

Shouldn't this be handled by GetSkipSides()?  Speaking of which, don't you need
to fix GetSkipSides()?	A lot of the places you set things to 0 may Just Work
if you do that...

I got up to the beginning of the nsLineLayout changes.	More later.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169014 [details] [diff] [review]
Patch rev. 2

>Index: layout/generic/nsIFrame.h
>+// Text reflow bit
>+#define TEXT_IN_WORD                                  0x04000000
>+
>+// First-letter layout bits
>+#define NS_FIRST_LETTER_FIRST_FRAME                   0x20000000
>+#define NS_FIRST_LETTER_LAST_FRAME                    0x40000000

Please document which frame types those bits apply to, since they're in the
"reserved for implementations" range.

Also, it may be worth putting those in the relevant header files (and creating
said header files if needed).  I'm ok with that happening in a separate bug,
but I really don't think that nsIFrame.h is the right place for
implementation-specific flags.

Also, please document what those bits mean.  They're not very
self-explanatory....  Knowing what they mean would make all the code that uses
them _much_ clearer.

>Index: layout/generic/nsBlockFrame.cpp

>+  if (nsLayoutAtoms::letterFrame == floatFrame->GetType()) {

Please document why floating letterframes that aren't the "first" or "last"
(whatever that means) should have zero margins on the relevant side?

>Index: layout/generic/nsBlockReflowState.cpp

>+  nsIFrame* floatFrame = aPlaceholder->GetOutOfFlowFrame();
>+  if (floatFrame && nsLayoutAtoms::letterFrame == floatFrame->GetType()) {
>+    fc->mIsCurrentLineFloat = PR_TRUE;

Please document why this is being done.

>@@ -816,25 +821,32 @@ nsBlockReflowState::FlowAndPlaceFloat(ns

>+  if (nsLayoutAtoms::letterFrame == floatFrame->GetType()) {
>+    // Keep all ::first-letter frames on the same line.
>+    if (!(floatFrame->GetStateBits() & NS_FIRST_LETTER_FIRST_FRAME)) {
>+      keepFloatOnSameLine = PR_TRUE;

I think the comment should be something like:

  If there are multiple ::first-letter frames, make sure to put
  all of them on a line together.  So if this isn't the first
  one, don't allow it to advance to the next band.

>Index: layout/generic/nsFirstLetterFrame.cpp

>+nsFirstLetterFrame::PaintSelf(nsPresContext*       
>+  nsFrame::PaintSelf(aPresContext,

Why not nsFirstLetterFrameSuper::PaintSelf?  If there is a good reason,
document it here.

Also, is there a reason you don't need to fix GetSkipSides?  If you did fix it,
would you still need to override PaintSelf?  Did you test this with other
things that use GetSkipSides (eg text-decoration in standards mode)?

>Index: layout/generic/nsLineLayout.cpp

>+  PRBool notSafeToBreak = CanPlaceFloatNow() ||
>+                          InWord() ||
>+                          (aFrame->GetStateBits() & TEXT_IN_WORD);

Are you sure aFrame is a textframe?  If not, this will do the wrong thing, no? 
I bet you need a frametype check here.

>Index: layout/generic/nsTextFrame.cpp

>+      // Trailing whitespace after ::first-letter punctuation
>+      if (aTextData.mFirstLetterOK) {

Could you document what exactly is being done here and why?

In particular, documenting nsLineLayout a tad (to say what the
"FirstLetterStyleOK" flag means) would go a long way toward understanding this
code.  Some explanation of why we're setting contentLen and wordLen and what
the mState change means would also help (perhaps document that bit a little
more extensively than is done now, say with an example?).

>+          aTextData.mX = 1;

Hmm.... This won't cause an unnecessary horizontal offset?

Sorry, I just don't know this code very well.  :( You may want to run this part
of the patch by rbs, perhaps....

>Index: layout/base/nsLayoutUtils.h
>+   * for ::first-letter's, see 

Remove the apostrophe (and perhaps the 's' too).

>Index: layout/base/nsLayoutUtils.cpp
>+// The following comemnt came from nsTextFrame.cpp, see the 

"comment"

>Index: layout/base/nsCSSFrameConstructor.cpp

>+    mPrefix(NS_FIRST_LETTER_PREFIX_EMPTY_OR_WS) {;}

Why the ';' in there?  Take it out, I think.

>+ * aIsFirstLetter is true if this text fragment belongs to the :first-letter

There is no such arg to this method....

>+    NS_ASSERTION(len == 0, "aText was not empty");

I think this would make more sense as an NS_PRECONDITION, with an unconditional
"else" at the end here and an assert that |s| is true.

This is as far as I got for now....  The letter frame construction and removal
logic is pretty hairy, and I'll need to be much more awake to review it.  :(

Also, I'd sort of appreciate not making all the whitespace changes for the
nsPresContext args, since I plan to remove those args altogether in the
not-too-distant future....

Comment 20

[Jungshik Shin]

rbs is not in Cc. perhaps, he's been watching it anyway. just to make sure.. 

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 169014 [details] [diff] [review]
Patch rev. 2

Mats, would you mind updating the patch with the documentation I asked for in
the first part of the review?  That would make the rest of this review a _lot_
easier... As it stands, I just spent an hour and a bit looking at the patch and
the code, and I still can't really sort out what's going on in a lot of it.

Marking r- for now, but again, that's not really an indication that there is
anything specific wrong, just that I can't tell.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (trailing punctuation)

I have missed the fact that trailing punctuation should also be included:
http://www.w3.org/TR/CSS21/selector.html#first-letter
"Punctuation ..., that precedes or follows the first letter should be
included,"

I have code that fixes that, just need to clean it up a bit.

The spec isn't clear on what to do when there is only punctuation,
e.g. <p>"</p>. So far I have assumed that it should be included
even when there is no first letter (this is the current behaviour),
but when I now read the spec again I get the impression that this
might not be what the spec author intended.

Comment 23

[Anne (:annevk)]

*** Bug 52109 has been marked as a duplicate of this bug. ***

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Comment 25

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.