Last Comment Bug 45091 - ::first-letter and openquote ::before should be together
: ::first-letter and openquote ::before should be together
Status: NEW
[CSS1-2.3]
: css1, css2, testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: P3 normal with 2 votes (vote)
: ---
Assigned To: Mats Palmgren (vacation)
:
Mentors:
: 52109 (view as bug list)
Depends on: 509685
Blocks: 2418 23605
  Show dependency treegraph
 
Reported: 2000-07-10 16:27 PDT by Erik van der Poel
Modified: 2011-03-16 18:16 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase #1 (3.04 KB, text/html)
2004-11-20 20:44 PST, Mats Palmgren (vacation)
no flags Details
Testcase #2 (float) (5.02 KB, text/html)
2004-11-20 20:45 PST, Mats Palmgren (vacation)
no flags Details
Patch rev. 1 (127.43 KB, patch)
2004-11-21 08:39 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 1 (diff -uw) (50.85 KB, patch)
2004-11-21 08:39 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 2 (143.41 KB, patch)
2004-12-17 21:57 PST, Mats Palmgren (vacation)
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
Patch rev. 2 (diff -uw) (66.37 KB, patch)
2004-12-17 21:59 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Testcase #3 (trailing punctuation) (2.79 KB, text/html)
2005-03-17 11:07 PST, Mats Palmgren (vacation)
no flags Details

Description Erik van der Poel 2000-07-10 16:29:15 PDT
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 2001-01-19 13:38:01 PST
remove Future for now.
Comment 2 Frank Tang 2001-03-29 21:21:23 PST
mark as moz1.0 for now.
Comment 3 Frank Tang 2001-05-07 23:43:18 PDT
erik resign. reassign all his bug to ftang for now.
Comment 4 Frank Tang 2001-05-08 00:29:28 PDT
move to future for now.
Comment 5 basic 2001-10-08 19:45:08 PDT
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 2001-10-08 19:57:38 PDT
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 2002-07-19 15:20:24 PDT
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 (vacation) 2003-07-14 04:35:16 PDT
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) 2004-03-06 06:31:22 PST
Isn't...

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

...more correct?
Comment 10 Mats Palmgren (vacation) 2004-11-20 20:44:23 PST
Created attachment 166640 [details]
Testcase #1
Comment 11 Mats Palmgren (vacation) 2004-11-20 20:45:06 PST
Created attachment 166641 [details]
Testcase #2 (float)
Comment 12 Mats Palmgren (vacation) 2004-11-21 08:39:05 PST
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.
Comment 13 Mats Palmgren (vacation) 2004-11-21 08:39:34 PST
Created attachment 166680 [details] [diff] [review]
Patch rev. 1 (diff -uw)
Comment 14 Boris Zbarsky [:bz] 2004-11-21 17:51:13 PST
Shared things can also go in nsLayoutUtils...
Comment 15 Mats Palmgren (vacation) 2004-12-17 21:57:55 PST
Created attachment 169014 [details] [diff] [review]
Patch rev. 2

OK, I added the "IsPunctuationMark" stuff to nsLayoutUtils.
Comment 16 Mats Palmgren (vacation) 2004-12-17 21:59:16 PST
Created attachment 169015 [details] [diff] [review]
Patch rev. 2 (diff -uw)
Comment 17 Boris Zbarsky [:bz] 2004-12-22 17:46:25 PST
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 [:bz] 2005-01-04 16:58:48 PST
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 [:bz] 2005-02-04 22:06:06 PST
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 2005-02-06 09:15:21 PST
rbs is not in Cc. perhaps, he's been watching it anyway. just to make sure.. 
Comment 21 Boris Zbarsky [:bz] 2005-03-04 20:07:21 PST
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 (vacation) 2005-03-17 11:07:16 PST
Created attachment 177749 [details]
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) 2005-04-06 02:55:12 PDT
*** Bug 52109 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.