Last Comment Bug 695538 - Missing trailing spaces during anonymous table boxes generation
: Missing trailing spaces during anonymous table boxes generation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Kang-Hao (Kenny) Lu [:kennyluck]
:
Mentors:
http://lists.w3.org/Archives/Public/w...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-18 15:41 PDT by Kang-Hao (Kenny) Lu [:kennyluck]
Modified: 2014-02-27 20:34 PST (History)
4 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (2.73 KB, patch)
2011-10-18 15:45 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
no flags Details | Diff | Splinter Review
proposed patch ver 2 (2.50 KB, patch)
2011-10-19 08:23 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
no flags Details | Diff | Splinter Review
proposed patch ver 3 (1.62 KB, patch)
2011-10-19 11:28 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
bzbarsky: review+
Details | Diff | Splinter Review
Comments addressed. With a reftest. (3.18 KB, patch)
2011-10-19 15:31 PDT, Kang-Hao (Kenny) Lu [:kennyluck]
kennyluck: review+
Details | Diff | Splinter Review

Description Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-18 15:41:50 PDT
There are cases where trailing spaces in a sequence of misparented table frames shouldn't be dropped (cases not covered by rule 1.3 of CSS2.1 17.2.1). Particularly, in the test page[1]

1)The space after "B" in the first example 
2)No box after the nested 'table-cell' and 'table-caption' case

[1] http://lists.w3.org/Archives/Public/www-archive/2011Oct/att-0016/anonymous-table-and-white-space
Comment 1 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-18 15:45:13 PDT
Created attachment 567912 [details] [diff] [review]
proposed patch

I am actually newbie to C++ so I don't know if this patch is OK performnace-wise.
Comment 2 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-19 08:23:52 PDT
Created attachment 568065 [details] [diff] [review]
proposed patch ver 2

Update comments that make sense.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2011-10-19 10:55:17 PDT
There shouldn't be any impact on performance here, I think.

Still recalling how this code works, but in the meantime... Is your patch basically equivalent to changing this condition:

          if (trailingSpaces ||
              spaceEndIter.item().DesiredParentType() != eTypeBlock) {

to this condition:

   if ((trailingSpaces && ourParentType != eTypeBlock) ||
       (!trailingSpaces && spaceEndIter.item().DesiredParentType() != eTypeBlock)) {

?  Seems like it is, followed by pulling that apart into two if statements which duplicate some code but maybe make the logic flow clearer?
Comment 4 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-19 11:26:51 PDT
(In reply to Boris Zbarsky (:bz) from comment #3)
> Still recalling how this code works, but in the meantime... Is your patch
> basically equivalent to changing this condition?

Yes, they are exactly the same.

> Seems like it is, followed by pulling that apart into two if statements
> which duplicate some code but maybe make the logic flow clearer?

Indeed (this also removes the need to change the assertion). I'll update the patch.
Comment 5 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-19 11:28:40 PDT
Created attachment 568128 [details] [diff] [review]
proposed patch ver 3
Comment 6 Boris Zbarsky [:bz] (TPAC) 2011-10-19 12:01:56 PDT
Comment on attachment 568128 [details] [diff] [review]
proposed patch ver 3

>+          // We drop the space if these are not trailing spaces (see case 2 above) or 

s/drop the space/drop the whitespace/ and perhaps "if these are not trailing spaces and the next item does not want a block parent"

>+          // if these are trailing spaces and aParentFrame is a tabulator container 

s/tabulator/tabular/.  But I'd be just as happy just saying "if these are trailing spaces and our parent type is not eTypeBlock" without reference to the spec here.  That makes it clearer what's really going on, I think.

r=me with the comment nits.

Can you add a reftest as well?
Comment 7 Boris Zbarsky [:bz] (TPAC) 2011-10-19 12:02:39 PDT
Oh, and a commit message for the diff would be good.  If you haven't picked up mq yet, this is a good chance to do that if you don't just want to commit the changeset locally.
Comment 8 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-19 15:31:43 PDT
Created attachment 568228 [details] [diff] [review]
Comments addressed. With a reftest.
Comment 9 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-19 15:43:24 PDT
(In reply to Boris Zbarsky (:bz) from comment #6)
> Comment on attachment 568128 [details] [diff] [review] [diff] [details] [review]
> proposed patch ver 3
> 
> >+          // We drop the space if these are not trailing spaces (see case 2 above) or 
> 
> s/drop the space/drop the whitespace/ and perhaps "if these are not trailing
> spaces and the next item does not want a block parent"

Done.

> 
> >+          // if these are trailing spaces and aParentFrame is a tabulator container 
> 
> s/tabulator/tabular/.  

Oops. Fixed.

> But I'd be just as happy just saying "if these are
> trailing spaces and our parent type is not eTypeBlock" without reference to
> the spec here.  That makes it clearer what's really going on, I think.

But then the comment pretty much just restates the conditional below (although I admit the code is much less confusing then that section of the spec as it is). I am leaving the CSS reference as it is for now.
 
> Can you add a reftest as well?

The first example in my test page added in layout/reftests/table-anonymous-boxes as 695538-1.html and 695538-1-ref.html . Do these need another review?

I might submit some of these tests to CSSWG as well.

(In reply to Boris Zbarsky (:bz) from comment #7)
> Oh, and a commit message for the diff would be good.  If you haven't picked
> up mq yet, this is a good chance to do that if you don't just want to commit
> the changeset locally.

Thank you for the instructions!
Comment 10 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-19 15:53:48 PDT
For future readers, this change is made for the following reasons (not just to match CSS 2.1, which might or might not be a valid one):

* It matches IE 9 for example 1,4,5 of the test page and WebKit for example 5,6,7,9. No example becomes worse in these cases in terms of interop.
* The behavior is more symmetric in the sense that if the FCItemIterator were moving in the reverse direction, the result is the same.
Comment 12 Boris Zbarsky [:bz] (TPAC) 2011-10-20 11:42:33 PDT
I'm sorry I wasn't cced; I missed comment 9....

Generally you do want review for tests.  In this case, the test has a typo: "cless" vs "class".  I believe it still failed before this patch because the whitespace was outside the <span cless="table"> so didn't participate in the wrapping process at all.

And as far as comment 10 goes, the old behavior is just clearly buggy.  No need to justify fixing it.  ;)

http://hg.mozilla.org/integration/mozilla-inbound/rev/e1fa0271b572 pushed to fix the test typo.

Thank you very much for finding the problem, and even more for the fix!
Comment 13 Kang-Hao (Kenny) Lu [:kennyluck] 2011-10-20 12:26:03 PDT
(In reply to Boris Zbarsky (:bz) from comment #12)
> Generally you do want review for tests.  In this case, the test has a typo:
> "cless" vs "class".  

Oops again.

> I believe it still failed before this patch because the
> whitespace was outside the <span cless="table"> so didn't participate in the
> wrapping process at all.

Yeah. 

> And as far as comment 10 goes, the old behavior is just clearly buggy.

Not to me! It is to me a very random algorithm of which I happened to find an edge case which didn't match the spec when I was trying to make sense it. I don't believe I would ever encounter this in the real world and I am as happy if CSS 2.1 matches Gecko before this patch, which has fewer conditionals.

> Thank you very much for finding the problem, and even more for the fix!

I have the feeling that I might get addicted to this and I am looking at the CSS 2.1 meta bug ;)
Comment 14 Boris Zbarsky [:bz] (TPAC) 2011-10-20 13:26:46 PDT
And pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/48967fae1b5c to make the test actually pass; the table needs to be display:inline-table for that.

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