Bug 24861 (nested-quotes)

Quotes nesting doesn't work [GC]

VERIFIED FIXED in mozilla1.8alpha1

Status

()

P2
normal
VERIFIED FIXED
19 years ago
8 years ago

People

(Reporter: pierre, Assigned: dbaron)

Tracking

({css2, testcase})

Trunk
mozilla1.8alpha1
css2, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch] hit during nsbeta2 standards compliance testing (py8ieh: make DOM tests) (py8ieh: fix test case), URL)

Attachments

(8 attachments, 15 obsolete attachments)

175 bytes, text/html
Details
1.46 KB, text/html
Details
1.87 KB, text/html
Details
1.14 KB, text/html
Details
4.60 KB, text/xml
Details
41.96 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
376 bytes, text/html
Details
1.17 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

19 years ago
The 'quotes' property allows you to specify the symbols you want for several 
levels of nested quoted strings. In Mozilla, the same first two symbols are used 
for all the levels of nesting.

The CSS spec is at http://www.w3.org/TR/html4/struct/text.html#edef-Q

A testcase will be attached.
(Reporter)

Comment 1

19 years ago
Posted file testcase
(Reporter)

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M16

Comment 2

19 years ago
Yow finally got to looking at this bug.  *sigh* While the CSS "quotes" part 
works, the FrameConstructor isn't actually tracking QuoteDepth. See:
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstru
ctor.cpp#725

Or look for the string:
PRUint32  quoteDepth = 0;  // XXX really track the nested quotes...

Yowch it just sets it to zero right now.  There is no place set up to track 
quotedepth or retrieve the value for these tricks.  It will always use the 
first quote set.

According to the CSS2 (which the quotes trick is from =) docs:
   http://www.w3.org/TR/REC-CSS2/generate.html#propdef-quotes
  "Note that this quoting depth is independent of the nesting of the source    
document or the formatting structure. "
That implies to me that a simple int set to zero for each page and then simply
inc'd and dec'd in the
  case eStyleContentType_OpenQuote:
  case eStyleContentType_CloseQuote:
  case eStyleContentType_NoOpenQuote:
  case eStyleContentType_NoCloseQuote:
cases would work.  It's just I dunno how to make and track that page variable
quoteDepth =)

Hopefully I at least saved somebody some searching about.
(Reporter)

Comment 3

19 years ago
Thanks much for the investigation.
I guess the quoteDepth can be tracked in the Document.
(Reporter)

Comment 4

19 years ago
With the comments above, the fix should be fairly easy.
Reassigned to attinasi to relax between more serious bugs.
Assignee: pierre → attinasi
Status: ASSIGNED → NEW
Target Milestone: M16

Updated

19 years ago
Status: NEW → ASSIGNED

Updated

19 years ago
Target Milestone: M16

Comment 5

19 years ago
A comment from Troy:

No, you can't store it in the document, because the quote depth is presentation 
specific. It would need to be stored in the frame construction code. 

Why are you working on nested quoted? We're explicitly not supporting it for 
version 1.0. It's not that hard to get it correct during the initial creation of 
the frame model, but when the content model changes you would need to reevaluate 
the nested quotes and that is going to take some work to get it right. 

Moving target milestone off a ways...
Target Milestone: M16 → M20

Comment 6

19 years ago
Marking REMIND on the advice of Troy - not going into Gecko 1
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → REMIND
Target Milestone: M20 → ---
As per meeting with ChrisD yesterday, taking QA.
Status: RESOLVED → REOPENED
QA Contact: chrisd → py8ieh=bugzilla
Resolution: REMIND → ---
Target Milestone: --- → Future
*** Bug 1061 has been marked as a duplicate of this bug. ***
Blocks: 7954
Keywords: correctness, helpwanted, testcase
Whiteboard: hit during nsbeta2 standards compliance testing
Summary: Quotes nesting doesn't work → Quotes nesting doesn't work [GC]
Additional testcases, with demonstrations of correct behavior:
<http://bugzilla.mozilla.org/showattachment.cgi?attach_id=4733>

Comment 10

19 years ago
Accepting to get bugzilla off my back. Will get to this post-RTM
Status: REOPENED → ASSIGNED

Comment 11

19 years ago
*** Bug 64907 has been marked as a duplicate of this bug. ***

Comment 12

18 years ago
See 87497 for the previous discussion.  It was suggested to move the discussion
here.

> What's the performance impact of this [87497] patch?

I haven't done measurements but I doubt it's much.  The loop climbs up the
document tree and therefore depends on the complexity of the document.  Each
round of the loop consists of a few simply pointer comparisons.

> Does it pass the "party tricks" part of >
http://www.bath.ac.uk/~py8ieh/internet/eviltests/content/2.html ?

No.  But I'm not sure this is a problem of the part I changed.  It probably
means that the wrong style sheet rule is used.

> Does it pass an equivalent test constructed in a random order using the DOM?

Don't know, do you have an example?

As I've said before, from what I read here my interpretation of the CSS2 differs
from what people here think.  I was assuming that nesting for the same tag is
relevant and not for all uses of quotes.  Also, the class is not taken into
account to compute the nestin elvel (should it)?

It probably would be good to get some definite answers on this and also some
test case for the DOM constructed text people were worried about.
I'll look into making DOM tests.
Keywords: qawanted
Whiteboard: hit during nsbeta2 standards compliance testing → hit during nsbeta2 standards compliance testing (py8ieh: make DOM tests)
See also the following comments from the bug I'm about to mark as a dup:

------- Additional Comments From Daniel Glazman 2001-06-29 03:17 -------

I think that your patch is incorrect. You assume that quotes are always
generated by the same tag, aren't you ?

This is false ; quotes are generated by elements that are selected by a rule 
containing a pseudo :before or :after with a 'content' declaration with
open-quote and/or close-quote values...

Hixie: yes, there is a performance hit in the proposed patch because it crowls
over all the element's ancestors each time a quote is inserted...
*** Bug 87497 has been marked as a duplicate of this bug. ***

Comment 16

18 years ago
> I think that your patch is incorrect. You assume that quotes are always
> generated by the same tag, aren't you ?

Yes, I've written this before here.  The way I've implemented it is how all
style sheets for SGML-like worked.  If the w3c's css2 defines it differently
(and the wording in the standard document is less than clear) it is invention. 
I can live with it but it should be made explicitly clear somewhere.
I have a problem with this bug : I don't see, given our implementation, any
solution for an implementation not having a high performance hit nor a high
footprint impact :-(

Comment 18

18 years ago
> I have a problem with this bug : I don't see, given our implementation, any
> solution for an implementation not having a high performance hit nor a high
> footprint impact :-(

I completely agree.  I've looked at a similar problem

  http://bugzilla.mozilla.org/show_bug.cgi?id=35768

and added some comments there.  I think this is a more generic problem which
requires decisions to be made ASAP.  The problem in general is:

  You have a tree.  Each node has a property which can depend on the
  properties of any or all of the nodes higher up in the tree.  The
  tree can be modified by adding more nodes or removing arbitrary nodes
  anywhere.  How to effectively track the track the property value?

The simplest solution is constant recomputation.  This cost would only hit those
who use the features requiring that.  The two cases I'm concerned about both
deal with i18n and I certainly don't want to discourage anybody from making the
publications more widely usable by punishing the reader.

One alternative is to store for each of the properties to track a pointer to the
node which last modified the property.  Any such pointer will point to the
parent, the parent of the parent, ...

For the quoting level it would point the the last node using quotes.  For the
lang pseudo-class it would point to the last node with a lang attribute definition.

The problem with this is that, beside from increasing the memory usage, all
routines modifying the tree have to be taught to update these pointers.  I'm not
yet familiar enough with the source tree to say whether all this code is located
in a few places to make this job feasable.  (Anybody want to comment on this?) 
The runtime implact would also exclusively be felt in tre modification code
(DOM).  The display code would be fast.  I think this is acceptable since DOM
code shouldn't be that fequent. 

I think this is a worthwhile approach but have one problem.  The requirement of
the pointer pointing to a parent, ... would mean that this mechanism could not
be used for counters (which I think are also not implemented and which would be
next on my list of things to do).  The counters don't have to be reset over
subtree boundaries and so finding the predecessor in the tree is not that easy.
 I'm not sure what impact (if any) this will have but it's worth mentioning.


I would really like to get some feedback on this.  What is the current feeling
of sacrifizing memory and/or runtime?

Comment 19

18 years ago
it'd probably cost a lot, but i think you could have a 'list of impressionable 
children'. either (1) containing the actual affected children or their 
(2) ancestor which is a child of the given node.  The second approach should 
cost less for storage but more for traversals while checking whether a node 
cared about the impression. (A)

assuming impressionable children know what attributes matter, it could be a 
(B) hash from attribute to nodelist so that traversal/checking is only done for 
truly affected nodes. [this adds mem cost (and list navigation) and decreases 
node traversal cost]

If parents aren't told that children are no longer interested then at some point 
all nodes could include all their children (2) or descendants (1) [yuck]. O(n)

A it's probably nearly impossible to decide when to remove a node. full rebuild 
[O(n^2)?]
Removal of nodes when updating a node:
1B go through correct hash and update all nodes O(N); go to each ancestor and 
remove all nodes which you just clobbered from the correct hash.
2B go through correct hash of children *recursive* and update all nodes for 
attribute; go to the parent and remove Hash{attribute=>self} - O(1)

So basically the only idea i'm listing, that makes any sense to me is 2B, adds 
hashes to arrays of children + time to traverse each of those down to the nodes 
that really care. -- This is probably to expensive both in time and space to 
consider (although it shouldn't take any space if no nodes need this feature)

The inheritance stuff is the RTL attribute, right? 

drepper:
HTML->BODY->b->c[sets/broadcasts]{pointer to P}->d->e->P[cares/listens].
So if i set e it would have to go through its ancestors looking for affected 
children. At C how does it know that P is a descendant of e? [my guess is go to 
P and then traverse up until reaching e or c] For revevant entries, it would 
remove the pointer to P, update P and add P to it's [e] own list?

hrm, so it's like 1B. (using less space but requiring acceptable upward 
traversals

Comment 20

18 years ago
> HTML->BODY->b->c[sets/broadcasts]{pointer to P}->d->e->P[cares/listens].
> So if i set e it would have to go through its ancestors looking for affected 
> children. At C how does it know that P is a descendant of e? [my guess is go
> to P and then traverse up until reaching e or c] For revevant entries, it
> would remove the pointer to P, update P and add P to it's [e] own list?

I'm not sure I really understand.  You want to add forward references which let
you find children depending on the current node easily.  While this is nice
for updating this isn't what I meant.  I was only concerned about speed in
the display code.  For this backward references are needed.  In your example:

  HTML   ->   nsnull
  BODY   ->   nsnull
  b      ->   nsnull
  c      ->   nsnull   (neither of these nodes has predecessor w/ the property)
  d      ->   &c
  e      ->   &c
  P      ->   &c

What this data structure buys you is a relative easy way to find out the value
for the current node.  In case of quotes count the number of back refs != nsnull
and add one to get the local value (could optionally be stored locally).  For
the :lang() pseudo-class you can for each node find out where it was last
changed (simply follow the back ref once).

Updating this data structure is a bit more complex:

  mybackref := current back reference
  myref := reference to current, changed node
  add all children to worklist
  for each node on worklist
    if backref in node == mybackref
      change backref to myref
      add all children of this node to worklist
    remove this node from the worklist

This you would have to run whenever the property of the node we are interested
in changes.  Removing node would require a similar algorithm.  Node that the
loop terminates as soon as it finds that the backreference is not the same as
for the changed node (which means another changed node is in between). This can
be a big win.  The costs therefore are:

  for display:
     for quotes:  level stored locally: O(1)
                  traverse to root:     O(n), in a real document rather O(ln N)
     for :lang(): O(1)

  for adding/removing an important node:
                  visit all children which point to removed node
                  (for quotes with locally stored level visit all children)
                  O(n)

  for adding/removing an unimportant node:
                  O(1).  if the parent is using the property use pointer to
                  parent, otherwise use same value as pointer in parent

From my point of view this is an acceptable solution.  It's not adding much
overhead (one pointer plus a member to store the property, language or quote
nesting level) and the important operations are fast.  It's only the insert
operations which take more time.  Note that while constructing the tree from
ground up one can easily keep track of the properties in O(1).


I might be missing something fundamental.
                  
Speaking of quotes implementation and language, don't forget what CSS 2 spec
says, and that adds another bit of complexity to the thing....

------------- QUOTATION BEGINS ------------- 

Note. If a quotation is in a different language than the surrounding text, it is
customary to quote the text with the quote marks of the language of the
surrounding text, not the language of the quotation itself.

Example(s):

For example, French inside English:
The device of the order of the garter is ?Honi soit qui mal y pense.? English
inside French:
Il disait: « Il faut mettre l'action en ? fast forward ?.»

A style sheet like the following will set the 'quotes' property so that
'open-quote' and 'close-quote' will work correctly on all elements. These rules
are for documents that contain only English, French, or both. One rule is needed
for every additional language. Note the use of the child combinator (">") to set
quotes on elements based on the language of the surrounding text:

[LANG|=fr] > *  { quotes: "«" "»" "\2039" "\203A" }
[LANG|=en] > *  { quotes: "\201C" "\201D" "\2018" "\2019" }

The quotation marks for English are shown here in a form that most people will
be able to type. If you can type them directly, they will look like this:

[LANG|=fr] > * { quotes: "«" "»" "?" "?" }
[LANG|=en] > * { quotes: "?" "?" "?" "?" }

------------- QUOTATION ENDS -------------

Comment 22

18 years ago
> Speaking of quotes implementation and language, don't forget what CSS 2 spec
> says, and that adds another bit of complexity to the thing....
> [...]

That's a different dimension (but exceptly what I meant with the reference to
:lang() in my description).  See

  http://bugzilla.mozilla.org/show_bug.cgi?id=35768

I've started looking a fixing this but it needs the same kind of decision to be
made.  The lang attribute of a node must be easily determinable.


One thing I'm not sure about (and the CSS spec doesn't say anything about) is
whether the different :lang() attributes for quotes mean different nesting
levels.  I.e., if you have

  <q lang="en">
    <q lang="fr">
      <q lang="en">

does the inner q have nesting level 2 or 3?  From your example and my
understanding it is 3.

Comment 23

18 years ago

Comment 25

18 years ago
I've attached two more files: attachment 49626 [details] is another test which also
includes a bit of DOM code.  Nothing complicated.

The attachment 49627 [details] [diff] [review] is a new patch.  It has not much in common with the
previous patch.  It adds the capability to track the usage of quotes.  I want to
elaborate a bit how I arrived at this patch.

I was trying to locate a data structure I can directly attach the information
to.  The nsIContent data is (from my understanding) not suitable.  The quotes
handling is part of the representation which should be separated from the data
itself.

So I was looking at the nsIFrame data.  Adding the info there (even with a
single bit) seemed to be a very nice solution.  But I found the maze of frames
not suitable.  First, not all nsIContent had a frame associated.  If
no-open-quote would be used no output is generated and therefore there is no
frame.   Second, looking for quotes earlier in the hierachy seemed very tedious
at best.  It's not a simple child-parent chain to look through.  Trying to
locate the primary frame for a nsIContent failed as well since at the time
CreateGeneratedFrameFor() is called this info seems not to be available for the
newly created frame.  I tried using GetPrimaryFrameFor() and this failed.

This is how I ended up using the PresContext.  It can be used to store
representation information and what I did is adding a table which reference the
nsIContent records which are using quotes.  This way I have to walk the
nsIContent chain up to the root and count the nesting level as the number of
nsIContent record referenced in the table.

Using a (hash) table this way seems somewhat overkill.  I could add the nesting
level of each element as data to the table and avoid climbing up to the root
each time.  This would work fine without DOM but with DOM this would require a
lot of extra work since inserting would require to update the info.  I can
change this if needed but I didn't find it necessary.

The patch I append has only one problem I know of: when remoing a nsIContent
element using quotes that record must be removed from the table.  I haven't been
able to find the place where I can do this.  I'd appreciate if somebody could
point me to the place.

Comment 26

18 years ago
nsCSSFrameConstructor::ContentRemoved could call into the PresContext to remove
the quote-content (RemoveQuoteContent). What happens then to cause all of the
frames with quotes to be reflowed and update their quote characters according to
their new nesting level?

Comment 27

18 years ago
> nsCSSFrameConstructor::ContentRemoved could call into the PresContext to remove
> the quote-content (RemoveQuoteContent).

I'll see whether I can figure something out.  Thanks,

> What happens then to cause all of the frames with quotes to be reflowed and
> update their quote characters according to their new nesting level?

I didn't find the reflow to be a problem.  Since this is about nesting removing
an upper-lvel quote automatically causes a reflow.  See the DOM code in the test
case I appended.  It's only the stale pointer in the hash table I have problems
with.

Comment 28

18 years ago
> Since this is about nesting removing an upper-lvel quote automatically causes a 
> reflow. 

Oh, right, of course. I look forward to seeing your next patch!

Comment 31

18 years ago
I've now added some code which cleans up the hash table.  It works fine.  I just
hope it's acceptable.  The new code is invoked whenever
nsCSSFrameConstructor::ContentRemoved() is called.  It has to walk the whole tree.

I just realized that this can be optimized a bit.  I could query the number of
quote nodes stored in the table.  If it is zero there is no need to run the new
code.  If the general concept is OK I'll implement this.  The current patch is
in attachment 49727 [details] [diff] [review].  An improved test in attachment 49726 [details].  Now you can create
multiple levels of quotes and see them disappear on one button press.

Anyhow, the only remaining possible problem I can think of is with DOM genrated
CSS rules.  I'll have to read a bit about DOM to see how to write such code.

Comment 33

18 years ago
I've attached yet another patch (attachment 49740 [details] [diff] [review]) which implements the
optimization I mentioned in the last comment.  With this optimization in place
the cleanup function isn't called if the document doesn't contain quotes.  I
found this the case most of the time when the ContentRemove function was called
which mainly happened for XUL contents.  This means that the performance
penalties are almost always near zero.
Keywords: patch, review

Comment 34

18 years ago
BTW: the document referenced the URL field

  http://www.mozilla.org/newlayout/testcases/layout/quotes.html

isn't correct, is it?  The text comes directly from HTML4 but the HTML spec
doesn't say anything about the default quotes used for "en-us".  The current
code (and I) expect the default quotes to be a single pair of ".

Maybe somebody should change the document referenced to contain

  <style type="text/css">
q { quotes: '"' '"' "'" "'" }
  </style>
Looks good! From looking at the patch I see two possible problems though.

The first is that it doesn't seem to handle (although I may be wrong) the case
where a single 'content' property has multiple quotes, as in:

 q:before { content: open-quote 'test' open-quote no-open-quote open-quote 'test'
                     close-quote no-close-quote; }
 q { quotes: ' <a ' ' a> ' ' <b ' ' b> ' ' <c ' ' c> ' ' <d ' ' d> ' ... ; }

The second is does this cope with siblings? As in,

   <q> Hello </q> <q> World </q>
...which should (with those CSS rules) be rendered as:

    <a test <b  <d test d> <c test <d  <f test f>

Also, how does this cope with the evil style sheet in
   http://www.bath.ac.uk/~py8ieh/internet/eviltests/content/2.html 
...? That tests most of the static aspects of this.

I have other tests here:
   http://www.hixie.ch/tests/adhoc/css/quotes/
...and here:
   http://www.hixie.ch/tests/adhoc/dom/css/quotes/I hope they help! I personally
*really* appreciate you working on this. You kick
ass! Good luck with those tests... :-D

Comment 36

18 years ago
Since Ulrich is working on this, shouldn't this get reassigned to him?

Comment 37

18 years ago
I'm not working on it at this time.  For several more weeks I have some other
high priority work to do.
*** Bug 117456 has been marked as a duplicate of this bug. ***
*** Bug 124609 has been marked as a duplicate of this bug. ***

Comment 40

17 years ago
Will Ulrich Drepper be working further on this? Otherwise, I'm willing to take a
shot at the remaining issues. It'd be a real shame to lose this patch to age,
IMHO :-/

BTW: Why is it marked qawanted?!

Comment 41

17 years ago
"Remaining issues" is nicely phrased :-).  You have to come up with something
completely different.  The nesting level as defined in CSS2 can depend on all
the previous text and not only that which is in the text hierachy higher (which
is what my last patch did).  You'll need a data structure which allows you to
efficiently find previous places in the do which used quotes.  In addition
you'll have to be aware of the issues DOM introduces.  At any time there can be
new text inserted dynamically so you cannot associate absolute quoting nesting
levels with the places stored in the said datastructure.

Finally, the whole new concept you come up with must be general.  Counters have
very much the same problem and should be implemented in the same way.

I'd love to see this implemented since I really really want counters to be
implemented so that it us finally possible to write documents naturally in
HTML/XHTML/XML.

I've tried to get some input from various mozilla people on how this all should
be implemented but didn't get much out of them.  The problem is that this is
really a new problem.

If you want to work on it please go ahead.  If you'd discuss your plans here
first I'm willing to provide you with the bit of insight I've gained while
thinking about this.  It's not that I forgot about implementing this, I simply
don't have the time it takes in the moment.
So, many parts of the CSS2 generated content chapter were pretty poorly thought
out aren't likely to be kept in future versions of CSS (considering the
requirement for implementations to pass CR, which CSS2 never really passed, so
in some sense, it is still in CR).  Perhaps no-close-quote and no-open-quote
should be implicit rather than explicit, and quoting depth could then be
associated only with ancestor/descendant relationships and not sibling
relationships?  (Are there any existing implementations that deal with nesting
of quotes properly for unmatched pairs?  Are there any existing implementations
other than ours?)  Then perhaps the quote depth could be tracked on the style
contexts?

Perhaps the first step would be to write a more easily implementable proposal
for how this *should* work.

Comment 43

17 years ago
Reducing the complexity of CSS2 is certainly an option.  But you have to be
aware that this does not cause the problem to go away.  Counters, as said
before, have a very similar problem.  And if we have to support the
functionality for counters, why not as well for nesting levels?

Having said this I certainly would like to get my last patch added as a first
step.  This will cover almost 100% of the cases quotes will be used.  Not every
thinks strangely enough to come up with nesting level requirements introduced by
siblings.

And to answer your question about other implementations: I don't know about any
myself.  Just to be sure I just dowloaded amaya 6.0 which has no support. 
Neither does have opera.
Comment on attachment 49740 [details] [diff] [review]
optimized removing of quote refrences

>+NS_IMETHODIMP
>+nsPresContext::IsQuoteContent(nsIContent *aContent, PRBool &aResult)
>+{
>+  ContentHash key(aContent);
>+  return mQuoteHash.Exists(&key);
>+}

The last line here should be replaced by:

aResult = mQuoteHash.Exists(&key);
return NS_OK;

and the caller should check NS_SUCCEEDED() of the result
rather than its value directly.  Either that or you should
use |NS_IMETHOD_(PRBool)|.

Also, instead of writing ContentHash, you could use
|nsVoidKey|.


However, on a design level, I'm not sure all of this is
necessary.  The parent relationships in the style context
tree are almost identical to those in the content tree.
The nasty issue is that the :before and :after pseudo-elements
get their own style contexts as children of the element's
style context, and aren't reachable without a full search of
the style context child list (which is unordered).  This could
be handled by setting a bit on the parent style context, but
then dynamic style changes would be difficult to handle.

So it makes me somewhat uncomfortable, but perhaps the general
idea is OK.  However, why put it on the pres context?  The
relationship between nsPresContext objects and
nsCSSFrameConstructor objects is 1:1 (there is always a 1:1
pres context / pres shell relationship (and nobody can remember
why they're separate or what the distinction is, but see bug
117568), the pres shell owns the style set, and the style set
owns the frame constructor), so the quote hash could be on the
frame constructor, and we could avoid adding yet more virtual
function calls.

Comment 45

17 years ago
dbaron@fas.harvard.edu:

> The last line here should be replaced by: [...]

OK, makes it more consistent.

> However, on a design level, I'm not sure all of this is necessary. [...]

I don't claim to know much about the moz internals but I did tinker quite a bit
around before arriving at this design.  It was a while back but I remember to
run into problems with not seeing certain nodes if I use anything else.  I see
your point with using nsCSSFrameConstructor (you want the hash table be moved
from nsPresContext into this class?) and this could be changed.

If you have any other opinions and ideas I'd like to hear them.  Especially if
you have any ideas on how the full problem and counters can be implemented.  As
I mentioned, my main interest today are counter.  The quotes were mainly a nive
way to test the lang support I wrote (and which is already accepted).

BTW: The file I mentioned in comment #34 still isn't fixed.
*** Bug 142216 has been marked as a duplicate of this bug. ***

Comment 47

17 years ago
This bug has been open for over 2 years now. It would be nice to have a 
foreseeable milestone set for this bug. 

Updated

17 years ago
Keywords: mozilla1.1

Comment 50

17 years ago
As pr. comment 41 here's my proposol for a design & implementation for this and
similar issues. 

The patch attached is a fully functional "proof of concept" patch, so if you
accept the design we already have a patch which you can pick apart :O) See
patch comments below the design comments.

Design comments:

I've attempted a "keep it simple, dammit" approach this time. 
For every Quoute and noQuote, a node is created in a doublelinked list in
nsCSSFrameConstructor. This list contains enough information to calculate the
quoting level. Furthermore, a ref to the /parent/ of the contentFrame is kept
in the list. This parent is used to keep the list sorted, lexigraphically by
position in the frame tree. 
Whenever a quote Frame is created, a node is inserted and the subsequent frames
are recalculated until the calculation match their former content. 
Whenever a quote Frame is destroyed, the node is removed from the tree.
(Actually, it's removed when it's parent frame is removed.) After all frames
that have been destroyed a removed, the List is recalculated and any changed
frames reset with the correct values.)

That's basically it. The insertion is, of course, the critical path,
performancewise. The insertion works by first checking if the node should be
appended, and if that's not the case, the insertion uses a binear search to
find the insertion point. Removal is done by simple transversal of the list.
The comparator for the frames needs to walk up the the tree, so it's O(N),
where N is depth of the tree. Thus, if M is the number of nodes in the List
Append: O(1)
Insert: O(N)*O(ln M)
Remove: O(M)
As all work is done in the frame construction, there's no rendering performance
hit.

Memorywise: A few bytes overhead, plus 32 bytes or so pr. node.

The performance hit when quotes are *not* used are construction/destruction of
one small object pr. pageload.

Patch nodes:
Almost the entire patch is in nsCSSFrameConstructor.cpp.

The QuoteList/QuoteListNode objects should maybe be split into a separate
object. nsCSSFrameConstructor.cpp is already pushing 14000 lines of code, so I
suppose added lines to this particular file is to be avoided if possible.

It would be possible to avoid roughly half the removal walkthroughs on the
QuoteList by setting a flag on nsIFrame only if the Frame is listed in the
list.

I've used the fact that child frames are always appended in
CreateGeneratedFrame(), using CreteGeneratedFrameFor(). 

The QuoteList::Destroy(nsIFrame* aFrame) is not a very good name. Suggestions
are welcome. What it does is destroying nodes with mParentFrame==aFrame.

The nodes are listed under their *parent frame*, as noQuoute's doesn't have a
Frame under which to list 'em.

Comment 51

17 years ago
Hmmm. Actually it fails the "party tricks" section of Ian's evil tests. I'll
look into that. 

Also, shouldn't the default stylesheet for html contain some quotes for <Q>?

There's also an error above: Append is in O(N) time, not O(1) (unfortunately),
as one comparison is needed to estiblish if the quote should be appended.

Comment 52

17 years ago
Posted patch Take 2. Corrected sign error. (obsolete) — Splinter Review
The party tricks from

http://www.bath.ac.uk/~py8ieh/internet/eviltests/content/2.html

revealed a "sign error" in my patch. This should now be
corrected. However, I believe that Ian Hixie has left an exercise to the reader
in his
"evil quote test"/"party tricks" :o). I don't think the party tricks should 
display 
"Isn't it wonderful to see CSS quotes work!!!". Instead, it should
look like this:

Isn't iNO!i
NO!!wonderful to
see CS
S quotes!!!

The rest is only interesting if you would like to know why I think so.

I've added the comments in {} to Ian's source where I believe Ian's 
comments are mistaken. I've also indented Ian's code to make the
nesting easier to follow.

  <STYLE>
     /* GENERAL */
     .test { margin-left: 2em; padding: 0.1em 1em 0.1em 1em; border-left: thin
solid; }

     .ok { quotes: '"' "\""; }
     .cool { quotes: "\201C" "\201D" "\2018" "\2019"; }

     .party1 * { display: inline; }
     .party1 .a { quotes: "Isn" "'" "t" "NO!" "NO!" " i"; }
     .party1 .b { quotes: "NO!!"       "NO!!"	 
			  "wonderful " "!!!"	
			  "to "        ""   
			  "see "       " work"	  
			  "C"	       " quotes"   
			  "S"	       " "; }
     .party1 .c { quotes: none; }

     .test .no-open:before { content: no-open-quote; }
     .test .open:before { content: open-quote; }
     .test .triple-open:before { content: open-quote open-quote open-quote; }
     .test .no-close:after { content: no-close-quote; }
     .test .close:after { content: close-quote; }
     .test .triple-close:after { content: close-quote close-quote close-quote;
}

  </STYLE>

<!--					     Isn't it wonderful to see CSS
quotes work!!! 
--><div class="test c party1"><!--	     0
  --><div class="a open close"><!--	     1 Isn		       
  --></div><!-- 			     0 '		       
  --><div class="a"><!--		     0			       
    --><div class="c open"><!-- 	     1			       
      --><div class="a open"><!--	     2 t		       
      --></div><!--			     2			       
    --></div><!--			     2			       
    --><div class="no-open close"><!--	     3 [NO!]		       
    --></div><!--			     2 i		       
  --></div><!-- 			     2			       
  --><div class="close"><!--		     2			       
  --></div><!-- 			     1 t {nothing *1*}		       
    
  --><div class="a no-open no-close"><!--    2			       
    --><p class="open close"><!--	     3 {"NO!" " i" *2*} 	       
       
    --></p><!-- 			     2			       
  --></div><!-- 			     1			       
  --><div class="b no-close"><!--	     1			       
    --><hr class="no-close"><!--	     0 [NO!!]		       
    --><br class="triple-open"><!--	     3 "", "wonderful ", "to " {"NO!!"
"wonderful " "to " *3* }
    --><br class="triple-open"><!--	     6 "see ", "C", "S"  
    --><div class="open close"><!--	     7 S {* patch error*}
      --><div class="close"><!--	     7			       
	--><div class="no-close"><!--	     7			       
	  --><div class="close"><!--	     7			       
	    --><div class="no-close"><!--    7			       
	      --><div class="close"><!--     7			       
	      --></div><!--		     6			       
	    --></div><!--		     5			       
	  --></div><!-- 		     4	quotes		       
	--></div><!--			     3 [ fail to]	       
      --></div><!--			     2	work {nothing *4*}	       
    
    --></div><!--			     1 !!!		       
  --></div><!-- 			     0 [NO!!]		       
--></div><!--				     0
-->

Comments:
*1*: This DIV inherits it's quotes property from the outermost div,
which has class "c", which has quotes:none. Thus, the "t" should
not appear.

*2*: This DIV inherits it's quotes property from the immediate parent,
which has class "a". As Ian's counting suggest, this DIV should use the
3rd quote-pair, which is "NO!" "i". 

*3* The first three open quotes from class "b" are "NO!" "wonderfull"
"to", not "" "wonderful" "to".

*4* This looks like an off-by-one. The third close quote in class b is
"", not "work" (which is the 2nd pair).
Attachment #87858 - Attachment is obsolete: true

Comment 53

17 years ago
Marc, can I take this bug? I tried to mail you, but the mail was bounced :-(
D'oh! I'll look into fixing the test "soon".
Assignee: attinasi → esben
Status: ASSIGNED → NEW

Comment 55

17 years ago
Thanks Ian :) Taking... this one seems a lot easier than the underline thing :-D
At least I don't have to worry about quirks...
Status: NEW → ASSIGNED
Whiteboard: hit during nsbeta2 standards compliance testing (py8ieh: make DOM tests) → hit during nsbeta2 standards compliance testing (py8ieh: make DOM tests) (py8ieh: fix test case)
Here is a version of the test that I believe is fixed:
   http://www.hixie.ch/tests/adhoc/css/quotes/002.xml
Let me know if you find any more problems...
FWIW there's a slightly simpler version of that test at:
   http://www.hixie.ch/tests/adhoc/css/quotes/001.xml

Comment 58

17 years ago
Ok, the new testcase works when the extra »}« in line 20 is removed :O) (My
patch passes the corrected test.) 

It looks nicer with an extra space after »t« in line 8, though.

It would be just great if somebody would either comment the design or review the
patch :-D

Comment 59

17 years ago
Are the other, older patches attached to this bug obsolete?

Updated

17 years ago
Attachment #49627 - Attachment is obsolete: true

Updated

17 years ago
Attachment #49727 - Attachment is obsolete: true

Updated

17 years ago
Attachment #49740 - Attachment is obsolete: true

Comment 60

17 years ago
I've marked all my patches obsolete.

Comment 61

17 years ago
I'm sorry to say but with the patch from attachment 88777 [details] [diff] [review] applied the test cases
don't look right.  Do I miss something?  This is on Linux (not that it should
make a difference).


One comment on the patch: we will have a very similar problem when handling
counters.  Wouldn't it be better to generate some more general data structures
which can be used for counters, too?

Comment 62

17 years ago
> I'm sorry to say but with the patch from attachment 88777 [details] [diff] [review] applied the test cases
> don't look right.  Do I miss something?  This is on Linux (not that it should
> make a difference).

Could you be more explicit (where and what)? It should just work<tm>. I may have
missed something. Two things to note: Currently, the party tests have errors and
the default stylesheet has no "quotes" property, so only quotes specified on a
stylesheet shows.

> One comment on the patch: we will have a very similar problem when handling
> counters.  Wouldn't it be better to generate some more general data structures
> which can be used for counters, too?

I'm looking into counters now. The idea was to factorize the QuoteList and
QuoteListNode so that QuoteList::Next, Prev, Insert etc. was put into a common
superclass, while QuoteList::Recalc and such creatures were kept in the
QuoteList class. I believe that a new specialization of this superclass would be
able to handle counters as well, though I may be wrong. Suggestions are welcome,
though. Already, I'm discovering that tracking the counter-reset,
counter-increment poses new and challeging issues :) Also a good name for the
superclass would be welcome... I seem to have inherited Leonardo of Quirm's
namegiving ability, unfortunately.

Thanks for trying out the patch :-)

Comment 63

17 years ago
Well, "does not work" means it is not much (if any) improvement over the code
before.  This is the output from Hixie's code referenced in comment #57:

  Isn'tFAIL!IsnFAIL!FAIL!FAIL!FAIL!wonderful to see CSS  quotes!!!

As mentioned, I've used the patch from your "Take 2. Corrected sign error"
attachment.  Is it possible that the patch is incomplete?

Comment 64

17 years ago
Posted file Corrected test.
>Well, "does not work" means it is not much (if any) improvement over the code
>before.  This is the output from Hixie's code referenced in comment #57:

>  Isn'tFAIL!IsnFAIL!FAIL!FAIL!FAIL!wonderful to see CSS  quotes!!!

>As mentioned, I've used the patch from your "Take 2. Corrected sign error"
>attachment.  Is it possible that the patch is incomplete?

The test is broken, not the patch. I'm uploading a corrected test... (the
difference: an extra "{" has been removed and a space has been inserted to make
it look nicer.

Comment 65

17 years ago
Oh, I forgot to add: You probably won't see any improvement over the previous
patch in the rendering department; the improvement is in the DOM handling, which
now works even though the affected quotes reside in another subtree in the DOM.
See the two "variations of 49726" above. Sorry I didn't make this clear before.

Comment 66

17 years ago
OK, an incorrect test case explains the problems.  In fact, it does now what it
is supposed to do.  Great work!


Wrt to reusing the code: what you describe is what I would have in mind myself.
 There is somewhere in moz probably some single/double linked list
implementation which you can use.  Don't ask me for names, I wouldn't know it.

Anyway, if you going to start testing some code I'm volunteering to test it. 
IMO counters are one of the most important missing features.  Which bug will you
add the patches to?

Comment 67

17 years ago
> OK, an incorrect test case explains the problems.  In fact, it does now what 
> it is supposed to do.  Great work!

Thanks :-D

> Wrt to reusing the code: what you describe is what I would have in mind 
> myself.

I'll take that means you approve. Good :-) I think it's the way to go, too, and
since nobody else seems inclined to comment, I'll go ahead and do counters the
same way :o)

> There is somewhere in moz probably some single/double linked list
> implementation which you can use.  Don't ask me for names, I wouldn't know it.

PR_LIST, I believe. I already use this :-P

> Anyway, if you going to start testing some code I'm volunteering to test it. 

Thanks. I can really use that :-) 

> IMO counters are one of the most important missing features.  Which bug will 
> you add the patches to?

Bug 3247, though it may be one or two weeks before I have any patches to share
:-/ Counters are a big one (and the reason why I started patching Mozilla in the
first place... my homepage doesn't have section numbers 'cause of this bug :) ).
It may be more if anything happens in this bug or bug 1777.
I could not find the extraneous '}' in
   http://www.hixie.ch/tests/adhoc/css/quotes/002.xml
...mentioned in comment 58. Could you be more explicit?

Also, the space should not be necessary as it is present in the " i" close quote.

Comment 69

17 years ago
I've copied and pasted the offending line below, but it wraps :-( It's quite
obvious if you just view the source in a maximized window. It the last character
on the line, beyond a lot of spaces. Perhaps searching for 20 spaces+} would
reveal it?

     .test { margin-left: 2em; }                                               
      }

As to the "t": Firstly, it *really* doesn't matter, it's pure aesthethics :-D
But the without any correction the 001.xml looks like this:

Isn't itwonderful to see CSS quotes!!!

as it should, but perhaps not as intended :o)
Testcases fixed, sorry about being slow. :-)

Updated

17 years ago
Blocks: 16206

Comment 71

17 years ago
When is this patch going to be checked in?  It looks from the comments that it
works.  From what I can tell all that is needed is changes to the default
stylesheets and a multilingual solution.  Please correct me if I'm wrong.  This
could be a useful patch.

Comment 72

17 years ago
The patch is awaiting reviews. I'd like a design review, but at least the
ordinary review and the superreview needs to be conducted. I requested a review
2002-7-01. If nothing happens for a while longer I'll try the next person on the
list. It's probably just the summer holidays for many people...

In particular, I would like to know whether the QuoteList class and relatives
should be put in a separate file. 

Ian's testcases works like a charm now, BTW.

P.S: Note that I made this patch as a proof-of-concept. It's just that it works
so well that I'd like to see it checked into Mozilla :-D

P.P.S: I've removed the help/qa-wanted keywords, as these make little sense now.
  The mozilla 1.1 is just plain unrealistic, now :o) The component should
probably be layout, as the style is computed find today, it's just not applied.
Keywords: helpwanted, qawanted
Target Milestone: Future → mozilla1.2alpha

Comment 73

17 years ago
Some early comments:

+  /**
+    * Removes Generated content from e.g. the QuoteList.
+    */
+  NS_IMETHOD GeneratedContentFrameRemoved(nsIFrame* aFrame) = 0;
+
[...]
+
+  /*
+   * If the frame contains generated context, remove it from
+   * the quoteList
+   */
+  if (mState & NS_FRAME_GENERATED_CONTENT) {
+    nsCOMPtr<nsIStyleSet> styleSet;
+    shell->GetStyleSet(getter_AddRefs(styleSet));
+    nsCOMPtr<nsIStyleFrameConstruction> construction;
+    styleSet->GetStyleFrameConstruction(getter_AddRefs(construction));
+    construction->GeneratedContentFrameRemoved(this);
+  }

No need for these. Use [ns]QuoteList (as in Rome) and move its handling into
nsFrameManager which handles other frame-related operations. There is a readily 
available method there, NotifyDestroyingFrame(), where you can add your cleanup 
when a frame goes away.

+  mQuoteList = new QuoteList();

rather than |new|ing unconditionally, you can make the first quote trigger the 
creation.

+struct QuoteListNode : public PRCList {
+  nsStyleContentType mType;
+  nsIFrame* mParentFrame;  // Used for ordering for quotes
+  PRUint32 mQuoteDepth;    // Depth AFTER quote
+  nsIContent* mContent;    // The content where this quote resides in the DOM.
+  const nsStyleQuotes* mQuotes;

1) Add comment why it is the parent frame that is kept
2) No need to keep nsStyleQuotes, you can get it from
   mParentFrame->GetStyleData() when need arises.

In fact, I would just use a struct like this:

+struct QuoteListNode : public PRCList {
+  nsStyleContentType mType;
+  PRUint32 mQuoteDepth;
+  // the textframe that contains the quote, it is null for noQuoute
+  nsIFrame* mQuoteTextFrame;
+  // keep the parent frame because noQuoute doesn't have 
+  // a corresponding textFrame. This is used for ordering quotes. 
+  nsIFrame* mParentFrame;
+}

This will avoid using objects that are ref-counted. (Otherwise you will have to 
explain why you don't hold a ref, etc. It is simpler not to bother here since 
you can get what you want from there.)

+        mQuoteList->Insert(node);
+        PRUint32 quoteDepth = mQuoteList->Calc(node);

*If* this design is retained, merge the logic of Calc() into insert. This way, 
the depth is kept in sync as soon as the node is inserted.

+      aNode->mQuoteDepth = (node->mQuoteDepth==0) ? 0 : node->mQuoteDepth - 1;
+      return node->mQuoteDepth - 1;

beware of troubles with PRUint32.

+void QuoteList::Destroy(nsIFrame* aFrame) {

Design: it looks like your list is actually a flattened tree. Try to switch to a 
kind of "quote tree". With that, a naming can be:
+void nsQuoteTree::DeleteSubtree(nsIFrame* aFrame)

Is FrameGreater() coded efficiently -- there might be more efficient 
alternatives.

Comment 74

17 years ago
> Some early comments:

Some late questions :o)

> No need for these. Use [ns]QuoteList (as in Rome) and move its handling into
> nsFrameManager which handles other frame-related operations. There is a
readily available method there, NotifyDestroyingFrame(), where you can add your
cleanup when a frame goes away.

Sound great, I'll do this shortly. 

>
> +  mQuoteList = new QuoteList();
>
> rather than |new|ing unconditionally, you can make the first quote trigger the
creation.

I suppose. Or I could just make QuoteList be a part of nsCSSFrameConstructor
object. It's only a few bytes, and quotes aren't that rare. I was newing because
I didn't wan't to mess with splitting (ns)QuoteList into a separate file, not
because it was good design. :) I went ahead and made the split. Not sure if I
should call it nsQuoteList, nsQuoteTree or nsGeneratedContent[List], though See
below.

[...]

> This will avoid using objects that are ref-counted. (Otherwise you will have
to explain why you don't hold a ref, etc. It is simpler not to bother here since
you can get what you want from there.)

Looks great. I've already made the neccessary modifications. (I still have a bit
of a hard time recognizing the refcounted objects...)
>
> +        mQuoteList->Insert(node);
> +        PRUint32 quoteDepth = mQuoteList->Calc(node);
>
> *If* this design is retained, merge the logic of Calc() into insert. This way,
the depth is kept in sync as soon as the node is inserted.

You're right I suppose. I was thinking that maybe I could make multiple
insertions, then do a final recalc when I was done. It doesn't work like that,
though, so I might as well merge them.

[...]

> beware of troubles with PRUint32.

Corrected.

>
> +void QuoteList::Destroy(nsIFrame* aFrame) {
>
> Design: it looks like your list is actually a flattened tree. Try to switch to
a kind of "quote tree". With that, a naming can be:
> +void nsQuoteTree::DeleteSubtree(nsIFrame* aFrame)

I'm not sure I understand this. Are you talking about something like this:

x --- x --- x --- x --- x
|                 |
x                 x
|
x

Where the vertical x's shared the same ParentFrame... But I can't see the
optimization (in either design or speed) so I guess I'm missing the point. Could
you please elaborate?

> Is FrameGreater() coded efficiently -- there might be more efficient alternatives.

It is coded for great efficiency, provided that I havn't missed some
information. The only information I'm using is that the frames are placed in a
tree with one common ancestor. I then determine the deepest common ancestor of
the two input frames, and determine which subtree is the former child of this
common ancestor. I don't think this can be done much better --- unless I can
somehow divine some more information... I unless I'm just missing something :o)

Thanks for your comments :-)

regards, Esben

Updated

17 years ago
Keywords: mozilla1.1 → mozilla1.3
What's the status of this bug ? it seems that a working patch is available. We
are seeing more and more nested quotes with weblogs now, it would be nice to
display nested quotes correctly for more readability. BTW, opera7b2 displays
nested quotes nicely ;-)
The status is that the patch needs work, I believe.

Comment 77

16 years ago
To be exact: The status is that while the patch works, it is a proof-of-concept
which needs to be grafted on to mozilla much more gracefully than it is now.
I've done most of this work, I just need to figure out exactly how to do this,
from comment 73:

> No need for these. Use [ns]QuoteList (as in Rome) and move its handling into
> nsFrameManager which handles other frame-related operations. There is a
> readily available method there, NotifyDestroyingFrame(), where you can add your
> cleanup when a frame goes away.

Also, I would still like some elaboration on the "flattened" tree comment, also
from comment 73... I just don't get what you mean, sorry.

I guess I've downpriotized this with the release of CSS 2.1 which I think
removed this functionality from CSS/HTML. Maybe Mr. Hixie could tell us where
this is headed in CSS3? Are quotes gone for good, or just out for regrouping?

Comment 79

16 years ago
Lynx and Amaya both render one level of nested quotes (i.e. "He said, 'Bye,' and
left."). For subsequent levels of quotes, Lynx alternates between " and ', and
Amaya just uses ' for all interior quotes. Neither one, I think, supports CSS2.

The HTML spec says UAs must render nested quotes in a language-appropriate
manner, so this is at heart an HTML issue, not a CSS issue. Mozilla claims HTML
support, so it should find a decent way to handle nested quotes even
(especially) in the absence of style rules.

I humbly suggest getting Mozilla to work with at least two levels of quotes,
since few authors will nest quotes much deeper than that, and worrying about the
CSS issues when CSS2.1 is finalized or someone sees the CSS3 generated content
module working draft.

Comment 80

16 years ago
XHTML 2.0 has the "quote" tag, which is different from the "q" tag. Bug 161463.
Not sure whether our plans for that need to mesh with this bug.
Keywords: mozilla1.3

Comment 81

16 years ago
Probably not. "quote" is invisible to the user, and doesn't generate quote marks
or anything like that (unless XHTML 2.0 changes by the time it becomes a
recommendation).
More to the point, <quote> is expected to be styled using 'quotes' and 'content'
by the content author in CSS environments such as Mozilla. So it depends on this
bug even more than HTML's <q> element does.

Updated

16 years ago
Blocks: 41368

Comment 83

16 years ago
I notice that while 
 q { quotes: '"' '"' "'" "'"; }
does not work as expected,
 q { quotes: '"' '"'; }
 q q { quotes: "'" "'"; }
works just fine.

This is not only a simple workaround for site authors,
but it seems to indicate that Mozilla itself is *already* 
counting element nesting, and that the former rule could
be handled internally as the latter rules.

Am I missing something?
You are missing something.

It is possible to have:

   a:before { content: open-quote; }
   b:before { content: open-quote; } b:after { content: close-quote; }
   c:after { content: close-quote; }

...and have:

   <a> ... </a>
   <b> ... </b>
   <c> ... </c>

...which should have the quotes of <b> nested inside the quotes opened by <a>
and closed by <c>.
*** Bug 213022 has been marked as a duplicate of this bug. ***

Comment 86

16 years ago
*** Bug 227001 has been marked as a duplicate of this bug. ***

Comment 87

15 years ago
This is an updated version of Esben's last patch.  It against the current tip
of the tree.  Compiles nicely on Linux and the smoke tests all work.  The tests
in the attachments work, too.

Additional change on top of updates:

~ the code for handling the list is in new files (.h and cpp).	None of the
functions need to be exported from the DSO/DLL.  The names are *Tree instead of
ns*List

~ the Calc function in private, it is automatically called on insert

~ the nsQuoteTreeNode structure is now as proposed in comment #73.

~ the nsQuoteTree object is now part of nsCSSFrameConstructor, not dynamically
allocated.  This increases the size of the class by only 4 bytes and the code
to handle the tree gets faster and smaller

~ the code in CreateGeneratedFrameFor got simplified.  The NoOpen/NoClose code
is not duplicated anymore.

~ I've cleaned up the code in nsFrame.cpp.  variable initialization and use are
moved closer and duplicate tests for NULL are removed

~ a few bug in the nsQuoteTree code are removed.  I've alse cleaned up that
code a bit

Of the points in comment #73 the only one not addressed is the first.  I tried
moving the code to nsFrameManager but this caused problems.  Double
free-related, probably.  I would need some pointers what is really meant here.
Attachment #88777 - Attachment is obsolete: true
Comment on attachment 138187 [details] [diff] [review]
Updated patch for tree on 2003-12-30

>-  // Get the view pointer now before the frame properties disappear
>-  // when we call NotifyDestroyingFrame()
>-  nsIView* view = GetView();

...

>+  // Get the view pointer now before the frame properties disappear
>+  // when we call NotifyDestroyingFrame()
>+  nsIView* view = GetView();

This seems to be missing the point of the comment.  |GetView| won't work
anymore once |NotifyDestroyingFrame| has been called.

>+NS_IMETHODIMP 
>+nsCSSFrameConstructor::GeneratedContentFrameRemoved(nsIFrame* aFrame) {

Could you follow local bracing style?  (Function opening brace on its own
line.)

It'll take me a bit of time to look through the quote tree code -- I may not
get to it immediately.
Attachment #138187 - Flags: review?(dbaron)

Comment 89

15 years ago
This patch moves the GetView() call to the beginning and changes various
functions to look more like the rest of the sources.  Compiled and tested.
Attachment #138187 - Attachment is obsolete: true

Comment 90

15 years ago
bryner helped adjusting the patch for a tree without nsIStyleSet.
Attachment #138257 - Attachment is obsolete: true
Comment on attachment 138591 [details] [diff] [review]
Update for nsIStyleSet removal

A few more comments, although I'm sure I'll have more (although I'm not sure
how much it's worth my looking at the current code given the first comment):

The use of |FrameGreater| seems incorrect.  It's going to fail whenever
multiple child lists are involved, and you really want content tree order
comparison anyway, not frame tree order comparison.  (I think our style tree /
frame tree relationship is backwards.)	We already have content tree order
comparison functions somewhere, but you'll probably have to be more careful
with :before vs. :after (although I haven't actually looked yet at whether you
were passing the generated content frames).

In nsCSSFrameConstructor.cpp, you changed a GetStyleQuotes call to the ugly
old-style GetStyleData call (probably bad merging).

The use of the name |QuoteTree| seems highly misleading for something that
isn't a tree.
See nsLayoutUtils::CompareTreePosition.  (However, handling :after might be tricky.)
nsLayoutUtils::CompareTreePosition currently does preorder comparison.  It could
be generalized quite easily (move the current function into
GeneralCompareTreePosition by adding two parameters for what to do when A is an
ancestor of B, and what to do when B is an ancestor of A).  The current function
could then call the general function with (-1, 1), a PostOrder comparison
function could pass (1, -1), and the code for this patch could call the general
code directly with (pseudo-type(A), pseudo-type(B)), where pseudo-type(:before)
is -1 and pseudo-type(:after) is 1.
I also think there's a good bit of similarity between quotes and counters and
the same mechanism might be able to work for both.

Also, I'm not comfortable with the handling of dynamic changes in this patch --
destroying everything isn't a good approach, and I'm also not sure it's done
enough, although I haven't looked closely.
(Or at least nsQuoteTree::Destroy should be renamed to
nsQuoteTree::DestroyNodesFor so I don't get confused about what's happening.)
Comment on attachment 138591 [details] [diff] [review]
Update for nsIStyleSet removal

>+      // Binary search.
>+      PRUint32 mask = 0x1;
>+      for (PRUint32 i = mSize; i > 1; i >>= 1) mask <<= 1;
>+      for (PRUint32 i = mSize & !mask; i>0; i--) {

I assume this is supposed to be ~ rather than !.

>+        node = Prev(node);
>+      }

Furthermore, I don't see how the code immediately following this handles size
not being a power of 2.  I'm tempted to rewrite this to avoid bit-twiddling.
Posted patch patch (obsolete) — Splinter Review
This makes a bunch of untested changes to the previous patch cleaning up a
bunch of my comments.  Calc and Recalc need a bunch of work still, at a minimum
to comply with the sentence "A 'close-quote' that would make the depth negative
is in error and is ignored (at rendering time): the depth stays at 0 and no
quote mark is rendered (although the rest of the 'content' property's value is
still inserted)."  I also think it would be better if we didn't recalc the
whole quote list twice when we remove a frame that has both an opening and
closing quote -- there are a bunch of possibilities for how we could do such a
thing (events, or perhaps BeginUpdate/EndUpdate).
It would be good to see some tests of the following (both static and dynamic):
 * elements whose :before and :after don't lead to balanced quotes
 * :before and :after containing more than one of open-quote, etc. (with both
   same-side and opposite-side quotes
Posted patch patch (obsolete) — Splinter Review
Better version of previous; still needs work.
Attachment #141666 - Attachment is obsolete: true
Posted patch patch (obsolete) — Splinter Review
This fixes the DOM issues in the previous patch, but I'm still not happy with
the dynamic change handling.
Attachment #141671 - Attachment is obsolete: true
FWIW, I think Hixie's test and the corrected version above are slightly buggy. 
I don't quite pass them -- I think the "work" is in the wrong place in the
'quotes' list so it doesn't show up, and there's also a double " " where there
should only be one.
I'll try to get to this asap, but it may take a week or so....
Comment on attachment 141700 [details] [diff] [review]
patch

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp

> nsCSSFrameConstructor::CreateGeneratedFrameFor(nsIPresContext*       

>+        nsQuoteListNode* node =
>+          new nsQuoteListNode(type, aParentFrame, aContentIndex);

Should probably handle OOM here.

>+        // "A 'close-quote' that would make the depth negative is in
>+        // error and is ignored (at rendering time): the depth stays at

There is some very similar logic in nsQuoteList::Recalc.  I think it would make
more sense to put all this code in a method on nsQuoteListNode that can be
called from both places.... (probably a method returnin const nsAFlatString& or
something along those lines).

>Index: layout/html/style/src/nsQuoteList.cpp

>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

This should use the MPL.

>+PRUint32
>+nsQuoteList::Calc(nsQuoteListNode* aNode)

It would be good to document what this function actually returns....  It looks
like it's returning the depth of aNode (which is the same as the depth before
aNode if aNode is opening and the depth after aNode if aNode is closing),
right?

>+void
>+nsQuoteList::Recalc(nsQuoteListNode* aNode, PRBool aContinue)

No one ever calls this with aContinue == PR_FALSE, as far as I can tell. 
Eliminate aContinue?

The code here that calculates quoteDepth is very similar to what Calc() does. 
Maybe we should factor it out?	This code also needs the same return value that
Calc() produces, I think.  The code as it is now gets prev->mQuoteDepth later
and decrementing it for close quotes, which is identical to the Calc()
algorithm, except Calc() gets the case of the node being mFirstNode right while
the code here ends up getting the quote depth off of prev even for mFirstNode,
which looks wrong to me.

So in fact, is there any reason not to make the following changes?

1)  Make this code stash away node->mQuoteDepth and then just call Calc()
2)  If the value of mQuoteDepth after Calc() is different from the value we
    stashed, then do the update
3)  Give Calc() an optional "prevNode" arg that we would pass here (Calc()
would
    just call Prev() when the arg is null).

>+#ifdef NS_DEBUG

#ifdef DEBUG, right?

>+nsQuoteList::PrintChain()

This method is never called.. I guess it's OK to leave it here for debugging
purposes, though....

>+nsQuoteList::~nsQuoteList()
>+{
>+  //Delete entire list
>+  if (!mFirstNode)
>+    return;
>+  for (nsQuoteListNode* node = Next(mFirstNode); node != mFirstNode;
>+       Next(mFirstNode))

So... node == mFirstNode is never true in this loop, right?  When do we delete
mFirstNode?  Also, that last clause of the for should probably be 

  node = Next(mFirstNode)

or something along those lines?

>+nsQuoteList::Insert(nsQuoteListNode* aNode)

>+      PR_INSERT_BEFORE(aNode, curNode);

if curNode == mFirstNode here, then we need to set mFirstNode = aNode, right?

>Index: layout/html/style/src/nsQuoteList.h

>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

MPL, please.

>+  nsQuoteListNode(nsStyleContentType& aType, nsIFrame* aPseudoFrame,

This should probably assert a few things in the body:

1)  mType is a quotes-related type (open, noopen, close, noclose)
2)  mPseudoFrame is non-null
3)  mPseudoFrame has the NS_FRAME_GENERATED_CONTENT state flag set

>+  PRUint32 Calc(nsQuoteListNode* aNode);
>+  void Recalc(nsQuoteListNode* aNode, PRBool aContinue);

Some documentation of what these functions do would be nice (in particular what
it is Calc() returns, like I said above).


>+  void RecalcChain(nsQuoteListNode* aNode) { Recalc(aNode, PR_FALSE); }

This seems to be unused.  Is it needed?
Attachment #141700 - Flags: superreview?(bzbarsky)
Attachment #141700 - Flags: superreview-
Attachment #141700 - Flags: review?(bzbarsky)
Attachment #141700 - Flags: review-
To clarify:

> >+      PR_INSERT_BEFORE(aNode, curNode);
> if curNode == mFirstNode here, then we need to set mFirstNode = aNode, right?

This is at the end of the binary search, not when appending.

Also, it would be good to include Esben's email with his name in the license
comments.
One other thing.  With the batching in that patch, we avoid having to recalc the
whole list every time we remove a frame on teardown, but we _do_ have to walk
the list each time to remove every frame.  So teardown is O(N^2) in the length
of the list.  I would think that we want to just call Clear() or something on
the list at teardown, walk it once destroying all the nodes, _then_ start
tearing down the frame tree.
Taking bug.
Summary: Quotes nesting doesn't work [GC] → Quotes nesting doesn't work [GC] [patch]
Summary: Quotes nesting doesn't work [GC] [patch] → Quotes nesting doesn't work [GC]
Whiteboard: hit during nsbeta2 standards compliance testing (py8ieh: make DOM tests) (py8ieh: fix test case) → [patch] hit during nsbeta2 standards compliance testing (py8ieh: make DOM tests) (py8ieh: fix test case)
I'm trying to clean up the calculation code so it doesn't have to go in multiple
places, and I'm probably going to change the mQuoteDepth to store the depth
*before* the node, so that it's easier to implement:

  A 'close-quote' that would make the depth negative is in error and is
  ignored (at rendering time): the depth stays at 0 and no quote mark is
  rendered (although the rest of the 'content' property's value is still
  inserted).
Posted patch patch (obsolete) — Splinter Review
This refactors the calculation stuff a good bit, and I think it addresses all
of bz's comments, but I'll double-check that it actually does so later.
Attachment #141700 - Attachment is obsolete: true
Posted patch patch (obsolete) — Splinter Review
This does address all of bz's comments.  I haven't yet tested it in a debug
build to make sure all of the assertions compile and are the right way around,
but if there's a problem I expect only minor changes.
Attachment #145719 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Priority: P3 → P2
Target Milestone: mozilla1.2alpha → mozilla1.8alpha
I'll attach a patch that fixes a few compilation problems with the assertions. 
None of the testcases hit any of my assertions, although a few of them (such as
http://www.hixie.ch/tests/adhoc/css/quotes/001.xml ) trigger:

###!!! ASSERTION: forget-word-frame: '(void*)aFrame == mWordFrames.PeekFront()',
file /builds/quotes/mozilla/layout/html/base/src/nsLineLayout.cpp, line 3097
Break: at file /builds/quotes/mozilla/layout/html/base/src/nsLineLayout.cpp,
line 3097
I'll try to look later tonight or tomorrow.
Comment on attachment 145731 [details] [diff] [review]
patch

>Index: layout/html/style/src/nsQuoteList.h

>+  // Quote depth this quote, which is always non-negative.

"before this quote", perhaps?

Why is this public if we have DepthBefore() (should we maybe make it protected
and add SetDepthBefore()?  Or just remove DepthBefore()?)

>+  void Calc(nsQuoteListNode* aNode);

Would still be nice to have a comment here explaining what that's supposed to
calculate....

r+sr=bzbarsky with those nits.
Attachment #145731 - Flags: superreview+
Attachment #145731 - Flags: review+
Attachment #145721 - Flags: superreview?(bzbarsky)
Attachment #145721 - Flags: review?(bzbarsky)
Fix checked in to trunk, 2004-04-12 14:52/53 -0700.
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago15 years ago
Resolution: --- → FIXED

Comment 118

15 years ago
Sorry if this has been reported, but I notice that quote-marks are now missing
in HTML documents that don't have the CSS quotes property set for <Q> elements.
I assume this is probably just a regression because a Mozilla .css file needs
to be updated to include the quotes attribute. Thought I better mention it in
case nobody has noticed this yet.
Comment on attachment 146116 [details] [diff] [review]
patch ua.css (checked in to trunk, 2004-04-14 13:13 -0700)

This gives nice en-US defaults but doesn't prevents authors from giving better
defaults.
Attachment #146116 - Flags: superreview?(bzbarsky)
Attachment #146116 - Flags: review?(bzbarsky)
Comment on attachment 146116 [details] [diff] [review]
patch ua.css (checked in to trunk, 2004-04-14 13:13 -0700)

Yeah, that looks good.	r+sr=bzbarsky.	Perhaps we should look into fixing bug
16206 too, now that we can.
Attachment #146116 - Flags: superreview?(bzbarsky)
Attachment #146116 - Flags: superreview+
Attachment #146116 - Flags: review?(bzbarsky)
Attachment #146116 - Flags: review+
Attachment #146116 - Attachment description: patch ua.css → patch ua.css (checked in to trunk, 2004-04-14 13:13 -0700)

Comment 122

15 years ago
*** Bug 268467 has been marked as a duplicate of this bug. ***
*** Bug 265681 has been marked as a duplicate of this bug. ***
*** Bug 271282 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Alias: NestedQuotes

Updated

15 years ago
Alias: NestedQuotes → nested-quotes
*** Bug 290495 has been marked as a duplicate of this bug. ***
*** Bug 304762 has been marked as a duplicate of this bug. ***

Comment 127

14 years ago
Has there been any movement on this bug since April of last year?  It's claimed 
to be Resolved Fixed, yet end users are not seeing the fixed behavior in public 
builds of Mozilla or Firefox.  I had reported this behavior as bug 304762 just 
recently, and it was marked duplicate of this one.  (I honestly did search the 
bugs DB, but I guess I didn't type in the magic keywords.)  Regardless, if this 
issue is resolved, why are current builds of Firefox still exhibiting the wrong 
behavior?
Robert, the bug has been fixed since April of last year.  You reported your bug
using Firefox 1.0.6, which is using a 1.7 stable branch build of the layout
engine.  That's layout engine code from April 1 of last year or so.

We've shipped a number of public alpha and beta builds of Mozilla and Firefox
that contain this fix (Mozilla 1.8 alphas 1, 2, and 3 come to mind, as well as
the Deer Park Alpha 1 and 2 builds of Firefox).  If you want to test layout
issues, please test in those builds, not in the very obsolete Firefox 1.0.x and
Mozilla 1.7.x builds.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.