Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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



Layout: Block and Inline
17 years ago
6 years ago


(Reporter: Erik van der Poel, Assigned: Mats Palmgren (vacation - back in August))


(Depends on: 1 bug, {css1, css2, testcase})

css1, css2, testcase
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [CSS1-2.3])


(3 attachments, 4 obsolete attachments)



17 years ago
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:


17 years ago
Target Milestone: --- → Future

Comment 1

17 years ago
remove Future for now.
Target Milestone: Future → ---

Comment 2

17 years ago
mark as moz1.0 for now.
Target Milestone: --- → mozilla1.0

Comment 3

16 years ago
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang

Comment 4

16 years ago
move to future for now.
Assignee: ftang → shanjian
Target Milestone: mozilla1.0 → Future


16 years ago

Comment 5

16 years ago
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

16 years ago
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?
Keywords: css2
Keywords: css1
Whiteboard: [CSS1-2.3]

Comment 7

15 years ago
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).
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:

CSS2.1 explicitly says that :first-letter is "innermost" which would mean:
FL = p:first-letter

Is this correct?  then what about margins, borders and such?
Assignee: shanjian → block-and-inline
Severity: normal → minor
Component: Layout → Layout: Block & Inline
QA Contact: petersen → ian
Blocks: 52109
Blocks: 23605

Comment 9

14 years ago


...more correct?
Keywords: qawanted
Summary: :first-letter and openquote :before should be together → ::first-letter and openquote ::before should be together
Assignee: core.layout.block-and-inline → mats.palmgren
Severity: minor → normal
Keywords: qawanted → testcase
Target Milestone: Future → ---
Created attachment 166640 [details]
Testcase #1
Created attachment 166641 [details]
Testcase #2 (float)
Created attachment 166679 [details] [diff] [review]
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.
Created attachment 166680 [details] [diff] [review]
Patch rev. 1 (diff -uw)
Blocks: 2418

Comment 14

13 years ago
Shared things can also go in nsLayoutUtils...
Created attachment 169014 [details] [diff] [review]
Patch rev. 2

OK, I added the "IsPunctuationMark" stuff to nsLayoutUtils.
Attachment #166679 - Attachment is obsolete: true
Attachment #166680 - Attachment is obsolete: true
Attachment #169014 - Flags: superreview?(dbaron)
Attachment #169014 - Flags: review?(bzbarsky)
Created attachment 169015 [details] [diff] [review]
Patch rev. 2 (diff -uw)

Comment 17

13 years ago
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

13 years ago
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
>+  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

13 years ago
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

>+  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 


>Index: layout/base/nsCSSFrameConstructor.cpp


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

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

Comment 21

13 years ago
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.
Attachment #169014 - Flags: superreview?(dbaron)
Attachment #169014 - Flags: superreview-
Attachment #169014 - Flags: review?(bzbarsky)
Attachment #169014 - Flags: review-
Created attachment 177749 [details]
Testcase #3 (trailing punctuation)

I have missed the fact that trailing punctuation should also be included:
"Punctuation ..., that precedes or follows the first letter should be

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.
Attachment #169014 - Attachment is obsolete: true
Attachment #169015 - Attachment is obsolete: true


13 years ago
No longer blocks: 52109

Comment 23

13 years ago
*** Bug 52109 has been marked as a duplicate of this bug. ***
QA Contact: ian → layout.block-and-inline
Depends on: 509685
You need to log in before you can comment on or make changes to this bug.