Closed Bug 1405929 Opened 7 years ago Closed 7 years ago

[BC] Join two cell's BEnd when two cells have same BEnd rowIndex. otherwise, it will introduce weird borders

Categories

(Core :: Layout: Tables, enhancement, P3)

Product:
Core
Component:
Layout: Tables
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: ywu, Assigned: ywu)

References

(Depends on 1 open bug,
URL
)

Details

Attachments

(7 files, 19 obsolete files)

test.html
623 bytes, text/html
Details
another testcase
1.09 KB, text/html
Details
one more test case from Bug 332740
1.75 KB, text/html
Details
an example that's broken by this patch
498 bytes, text/html
Details
a larger set of examples
1011 bytes, text/html
Details
0001-Bug-1405929-Make-sure-that-we-join-two-BEnd-borders-.patch
2.18 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
reftest
6.33 KB, patch
ywu
: review+
Details | Diff | Splinter Review 
In this bug, I would like to work on fixing Testcase #2 from bug 332740.
Attached file test.html — 

    
    

    
  

      
      
      
      

        Attached file
          
        another testcase
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        test case from Bug422955 (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        remove buggy part (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  


  

        Attachment #8926743 -
        Attachment is obsolete: true

    
    

    
  


  

    
    

    
  
Comment on attachment 8927729 [details] [diff] [review]
Part-1-Rowspan-on-border-collapsed-borde.patch

Hi David,

I would like to remove these codes and here is my reason. 
First, these lines cause the wrong behavior of drawing the border(extra lines were shown. see the attachment for test cases). 
Second, the comments is really unclear why we need these codes. 
Third, |ResetBStartStart| is badly written(There are three unused cases in this function. case eLogicalSideBStart, eLogicalSideIEnd, eLogicalSideIStart).
And finally, there are no reftests to secure these codes(not clear why we need these codes). 

So I think we might give this a try and let the trains tell. 
How do you think?

        Attachment #8927729 -
        Flags: review?(dbaron)

    
  

        Attachment #8927730 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8926762 [details] [diff] [review]
reftest

Comment 5 has tested the reftest w/o the patches on try server.

        Attachment #8926762 -
        Flags: review?(dbaron)

    
    

    
  
btw if we land these and the trains have no complaints, we might need to examine the use of |mBStartStart|.

    
    

    
  
(In reply to Ya-Chieh Wu from comment #9)
> I would like to remove these codes and here is my reason. 
> First, these lines cause the wrong behavior of drawing the border(extra
> lines were shown. see the attachment for test cases). 

So this is a decent reason to investigate this code, but it's not alone sufficient reason to remove it.

> Second, the comments is really unclear why we need these codes. 

This isn't a good enough reason.  Some of our code doesn't have good enough comments, but that doesn't mean we get to break web content.

> Third, |ResetBStartStart| is badly written(There are three unused cases in
> this function. case eLogicalSideBStart, eLogicalSideIEnd,
> eLogicalSideIStart).

This also isn't a good enough reason.  Yes, there's only one caller and it passes eLogicalSideBEnd.  That justifies simplifying the code, but not necessarily removing it.

It also hints that, perhaps, the reason the code exists is to handle cases that involve multiple rowgroups in a table, something I suspect we have very few tests for.  This includes, for example, multiple tbody elements in a table, or a mix of tbody/thead/tfoot elements (only one thead or tfoot is normally allowed).

> And finally, there are no reftests to secure these codes(not clear why we
> need these codes). 

This also isn't a good enough reason.  The code was probably written before we had reftests.

If you want to justify removing it, you should:
 * look at the history of the code to find when it was introduced, and why.  You'll probably need https://github.com/mozilla/gecko-dev/ for that since it has history from CVS
 * if you can find them, look at the testcases that justified the addition of the code.

If not, however, you could try writing testcases that demonstrate that the code is needed (for example, by testing dynamic changes where a change would need to be stored in the data of a different rowgroup), or by reasoning about the logic of the code as a whole to explain what this code was intended to do and why it's no longer needed.

    
    

    
  
Comment on attachment 8926762 [details] [diff] [review]
reftest

>+<title>bug1405929 reftest</title>

This should probably say "Bug 1405929 reftest reference"

>+<title>bug1405929 test</title>

This should probably say "Bug 1405929 reftest"

>@@ -1,8 +1,9 @@
>+== bug1405929.html bug1405929-ref.html
> == bug1375518.html bug1375518-ref.html
> == bug1375518-2.html bug1375518-ref.html
> == bug1375518-3.html bug1375518-ref.html
> == bug1375518-4.html bug1375518-4-ref.html
> == bug1375518-5.html bug1375518-5-ref.html
> == bug1379306.html bug1379306-ref.html
> == bug1394226.html bug1394226-ref.html
> != bug1394226.html bug1394226-notref.html

This should be sorted numerically after the lower bug numbers.

You should feel free to land this test now, with "fails ==" rather than just "==", assuming that it fails today, which it should.

If it doesn't fail today, then you need to fix it so that it does!

        Attachment #8926762 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8927729 [details] [diff] [review]
Part-1-Rowspan-on-border-collapsed-borde.patch

This might be OK, but it needs more justification.  See comment 12.

        Attachment #8927729 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8927730 [details] [diff] [review]
Part-2-Remove-unused-function.patch

This might be OK, but it needs more justification.  See comment 12.

        Attachment #8927730 -
        Flags: review?(dbaron)

    
    

    
  


  
Hi David,

thx for the suggestions on comment 12. 
I have followed the hints and could not find the history of the test case for the codes that I was trying to remove. I might miss something. However, I have written a band-aid first which can at least fix a few test cases that I took from bug 332740.

In this patch, I check if the two bottom borders are in the same row(right now it only checks color, width, style), then we can draw them together without starting a new segment[1]. Otherwise, we shouldn't bring them together.

However, this is just a band-aid for now. I find that the codes that I was trying to remove are still introducing some weird behaviors.(my previous patch can fix the weird behaviors) But as you said, I have to go over the logic of the code as a whole and give a reasonable explanation for removing them. I will follow up.
 
[1]somthing relate to the new segment is using here
https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#7942

try looks ok:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92507b417830b1c746057a046726df0f84070036

how do you think?

        Attachment #8925840 -
        Attachment is obsolete: true

        Attachment #8927729 -
        Attachment is obsolete: true

        Attachment #8927730 -
        Attachment is obsolete: true

        Attachment #8929424 -
        Flags: review?(dbaron)

    
    

    
  
(In reply to Ya-Chieh Wu from comment #16)
> I have followed the hints and could not find the history of the test case
> for the codes that I was trying to remove. I might miss something.

Given that the code dates back to https://github.com/mozilla/gecko-dev/commit/679c5752025fb108be2fe98f6e805409b223264b (the SetTopStart(PR_FALSE) call in nsTableFrame.cpp), I agree you're unlikely to find testcases...

    
    

    
  
Hi David,

Add a bit more explanation. 
Attachment #8929424 [details] [diff] is similar to https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6420
I try to make sure that if we set |mBStartStart| to false, it has to be two continuous borders(on the same row index and next to each other) with the same style, border size, and color.  

I think the usage of |mBStartStart| is not very straightforward and it introduces some rowspan bugs.
We might need to consider if it is necessary to remove it or maybe at least rename it?
I will create a follow-up bug to handle it somehow.

How do you think?

    
  
Flags: needinfo?(dbaron)

    
    

    
  


  
Hi David,

Sorry for submitting this patch again but I would like to justify the code here. And trying to remove them. 

I remove these lines because just look at the logic of the code; it doesn't look like it's needy and remove these lines can resolve the row span bugs as the attachment test cases.

Here is what I think,
The code here tries to connect the cell's BEnd border if they can be drawn together(same style, width, color). However, it doesn't look like that we have to check the case when the cell to the right has a row span. Because this case should be covered already when we go over the cells from left to right when we try to check the BEnd border[1][2]. Look at the [1][2]; it uses |startSeg| to check if they can or can't connect the BEnd.

[1] https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6301,6331
[2]https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6358,6414

        Attachment #8929424 -
        Attachment is obsolete: true

        Attachment #8929424 -
        Flags: review?(dbaron)

        Attachment #8930839 -
        Flags: review?(dbaron)

    
    

    
  


  
this code is unused after the first patch.

        Attachment #8930841 -
        Flags: review?(dbaron)

    
    

    
  
David, how do you think?
Flags: needinfo?(dbaron)

    
    

    
  


  
Without this patch, the border here is a single dotted border, but with the patch, it's two distinct dotted borders.

(Note, however, that this doesn't work correctly today at the bottom edge of a table.)

    
    

    
  


  
Here's a larger set of examples that I was looking at.

    
    

    
  
Comment on attachment 8930839 [details] [diff] [review]
Part-1-Rowspan-on-border-collapsed-borde.patch

So the above example disproves the claim that the code doesn't perform a useful function.  (That doesn't mean that we couldn't do it a better way -- this code is very confusing and also pretty broken.)

I'd also note that in all your code removals, you should probably remove a line of whitespace so that there isn't an extra blank line where you removed the code, relative to the spacing style that was present before the removal.

        Attachment #8930839 -
        Flags: review?(dbaron)

    
    

    
  
(Also note that our dotted border rendering in border-collapse is pretty awful.  The corners are bad, we're using square dots instead of circular ones, and there's a shift in alignment between the corners and the center.)

    
    

    
  
Also, a bunch of these examples should probably be turned into reftests.  (Though some of them would today be failing reftests.)

    
    

    
  
Thanks for the test cases, but there is one thing that I am not sure of.
I wonder what is the right behavior or reasonable behavior for the corner that connects to two borders.
It seems that we sometimes use a single dot, but sometimes we use two distinct dots.
It seems that chrome and safari use two distinct dots.
I wonder a single dot is what we want? (for consistency)

How do you think?

    
  
Flags: needinfo?(dbaron)

    
    

    
  
I think the right behavior is that dotted borders should be consistently spaced for as long a segment as possible.  We try to do that, although attachment 8931155 [details] shows we don't quite succeed.
Flags: needinfo?(dbaron)

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
  

      
      
      
      

        Attached file
          
        bug1405929.html (obsolete)
        — 
      

    


  
one more test case

    
    

    
      
  

    
  

        Attachment #8931591 -
        Attachment is obsolete: true

        Attachment #8931591 -
        Flags: review?(dbaron)

    
  

        Attachment #8931592 -
        Attachment is obsolete: true

        Attachment #8931592 -
        Flags: review?(dbaron)

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8931595 -
        Attachment is obsolete: true

        Attachment #8931595 -
        Flags: review?(dbaron)

    
  

        Attachment #8931596 -
        Attachment is obsolete: true

        Attachment #8931596 -
        Flags: review?(dbaron)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
Sorry about the pushes of mozreview. I am learning to use it.

I would like to explain the patches a little bit.
The logic of the bc border here is hard to explain so I would try to make it clear.

So as I use the debugger to check [1][2] where a cell was attempting to join it's IStart side's BEnd border.
And I realize that for [1][2] if a cell has a row span, it's BEnd border cannot be joined to its IStart side's cell because the cell to the IStart side hasn't been processed yet. (the iterator is iterating row by row) That's why we need this function.
However, we forget to check when we want to join two BEnd borders; the two borders have to be on the same row.
This introduces the weird behavior as attachment 8915444 [details], 8925839, 8931593...

So in the first patch, I make sure that two BEnd have the same row Index.
For part 2, we have to get the right cell from the right row group. (same as what we did here [3])
For part 3, I try to put down some comments and refactor a bit, but my wording might be bad, so maybe you can help to put them in a better way.

Finally, for attachment 8931155 [details], the third case that we don't do it right.(we don't draw them together)
And I also use a debugger to check the row index and col index that we pass in in that case.
It seems that we do pass in the right index, but for the border at the BEndMost, we store it a bit different way.
We have to further modify the |ResetBStartStart| but I think we don't necessarily have to do it here.

thanks for helping


[1]https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6301,6331
[2]https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6358,6414 
[3]https://searchfox.org/mozilla-central/source/layout/tables/nsCellMap.cpp#963,977

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8931599 [details]
Bug 1405929 - Part 1 : Make sure that we join two BEnd borders when they are on the same rowIndex.

https://reviewboard.mozilla.org/r/202740/#review209580

I'd like to see another patch, given the last of the comments below.

::: layout/tables/nsTableFrame.cpp:6457
(Diff revision 1)
>          info.SetInnerRowGroupBEndContBCBorder(nextRowGroup, ajaInfo.mStartRow);
>          gotRowBorder = true;
>        }
>      }
>  
>      // see if the cell to the iEnd side had a rowspan and its bEnd-iStart border

could you fix this "bEnd-iStart" to just say "bEnd"?  It's just talking about the border on the block-end side.

::: layout/tables/nsTableFrame.cpp:6460
(Diff revision 1)
>      }
>  
>      // see if the cell to the iEnd side had a rowspan and its bEnd-iStart border
>      // needs be joined with this one's bEnd
>      // if  there is a cell to the iEnd and the cell to iEnd side was a rowspan
>      if ((info.mNumTableCols != info.GetCellEndColIndex() + 1) &&

Maybe create a variable:
  const auto nextColIndex = info.GetCellEndColIndex() + 1;
rather than repeating the expression four times?

::: layout/tables/nsTableFrame.cpp:6462
(Diff revision 1)
> +        (lastBEndBorders[info.GetCellEndColIndex() + 1].rowIndex ==
> +         info.GetCellEndRowIndex() + 1)) {

I'm having trouble understanding this condition.  It looks at first glance like a condition that should never be true ("the starting row index of the cell to the right is the same as the ending row index of this cell, plus one").

Did you mean to test whether the bottom of this cell is the same row index as the bottom of the cell to its right?


It would also help if the commit message was clearer in describing what change you're trying to make.

        Attachment #8931599 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8931590 [details]
Bug 1405929 - Part 2 : fix the wrong row index.

https://reviewboard.mozilla.org/r/202726/#review209584

This is mostly reasonable.  However, nsTableCellMap::ResetBStartStart looks like it uses aRowIndex in three places.  The first two of them are indices into aCellMap (an nsCellMap, which is for a single row group, as compared to the nsTableCellMap, which is for the whole table).  The third one, however, is an index that looks like it should be relative to the whole table rather than the cell map.  So this patch fixes the first two uses and breaks the third.  You should adjust it to handle all three correctly.

Also, please include *in this patch* two reftests:
 - one that failed without this version of the patch and passed with it
 - one that passed before the patch and with the revision as I describe, but fails with this version of the patch

        Attachment #8931590 -
        Flags: review?(dbaron) → review-

    
    

    
  


  

        Attachment #8930839 -
        Attachment is obsolete: true

        Attachment #8930841 -
        Attachment is obsolete: true

        Attachment #8931590 -
        Attachment is obsolete: true

        Attachment #8931597 -
        Attachment is obsolete: true

        Attachment #8931599 -
        Attachment is obsolete: true

        Attachment #8932024 -
        Attachment is obsolete: true

        Attachment #8931597 -
        Flags: review?(dbaron)

        Attachment #8932024 -
        Flags: review?(dbaron)

    
    

    
  
There are too many patches here. I want to split the handling of |ResetBStartStart| to another bug.
Summary: [BC] rowspan on border-collapse is wrong. → [BC] Join two cell's BEnd  when two cells have same BEnd rowIndex. otherwise, it will introduce weird borders

    
    

    
  
see Bug 1421887 for handling of |ResetBStartStart|
See Also:  → 1421887

    
    

    
  


  

        Attachment #8926762 -
        Attachment is obsolete: true

        Attachment #8931593 -
        Attachment is obsolete: true

        Attachment #8933181 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8933173 [details] [diff] [review]
0001-Bug-1405929-Make-sure-that-we-join-two-BEnd-borders-.patch

Hi David,

In this patch, I add more commit messages to explain the patch.
I also want to note that I move the handling of |ResetBStartStart| to Bug 1421887 since there are many tests here and I want to make a bit clear.


Try:
With this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aa9e96ab68459bc4158e4f7a2b20967b64b46e9&selectedJob=148677107

Without this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08548c965982e65b39f093eef3fe26f13e90acdf&selectedJob=148678621

        Attachment #8933173 -
        Flags: review?(dbaron)

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #44) 
> Did you mean to test whether the bottom of this cell is the same row index
> as the bottom of the cell to its right?

Yes. The naming or usage here in |clacbcborder| might be a little bit confusing.
We store |lastBEndBorder.rowIndex| as |info.GetCellEndRowIndex() pluse 1|.
Like https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#6347

    
    

    
  
So this function is extremely difficult to understand.  I think it could use some comments explaining why it does what it does.  I suspect that my attempts to understand just a piece of it might have led to some incorrect assumptions.

Some things that I think are worth explaining in comments (if they're even true -- I'm not sure):

 * lastBlockDirBorders is the most recent inline-side border processed along each column boundary.  It exists only to determine whether the next inline-side border starts a new segment (via startSeg).

 * lastBEndBorders is the most recent block-end border processed at the block-end edge of some cell in each column.  These are only stored to determine corner ownership in order to (in turn) determine whether the next inline-side border starts a new segment.  (It seems like it stores an extra entry at the end of the array that's never used !?)

 * lastBStartBorder and lastBEndBorder are used to track whether block-side borders need to start a new segment (via startSeg).

Are these statements correct?

Do BCCellBorder::rowIndex and rowSpan refer to the row-index and rowspan of the cell whose border they are?  (The cell on which side?)

    
    

    
  
(In reply to Ya-Chieh Wu from comment #51)
> (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #44) 
> > Did you mean to test whether the bottom of this cell is the same row index
> > as the bottom of the cell to its right?
> 
> Yes. The naming or usage here in |clacbcborder| might be a little bit
> confusing.
> We store |lastBEndBorder.rowIndex| as |info.GetCellEndRowIndex() pluse 1|.
> Like
> https://searchfox.org/mozilla-central/source/layout/tables/nsTableFrame.
> cpp#6347

A more reliable link here is:
https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/layout/tables/nsTableFrame.cpp#6347
since it will still work when the code next changes.

I guess that answers the "the cell on which side" question, though.

    
    

    
  
Comment on attachment 8933173 [details] [diff] [review]
0001-Bug-1405929-Make-sure-that-we-join-two-BEnd-borders-.patch

OK, r=dbaron on the code change here.

However, the commit message still needs work.  I think most of the message should go (it has too much detail), but you should revise the code comments around "see if the cell to the iEnd side" to explain in a single sentence why this code is needed.  Maybe something like "We normally do this work when processing the cell on the iEnd side, but when the cell on the iEnd side has a rowspan, the cell on the iStart side gets processed later (now), so we have to do this work now."

Clearing review because I'd like to look at the revisions.

(It would also be nice to have a separate patch improving some of the other comments; see my comment 52.)

        Attachment #8933173 -
        Flags: review?(dbaron)

    
    

    
  
(I'm hoping that sentence is the reason, but if I'm wrong, you should definitely correct me!)

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #52)
> So this function is extremely difficult to understand.  I think it could use
> some comments explaining why it does what it does.  
> 

Yes, I have the same feelings. I think it might be worth filing a bug to address
those naming or add more comments. I will put this on my to-do.

 
>  * lastBlockDirBorders is the most recent inline-side border processed along
> each column boundary.  It exists only to determine whether the next
> inline-side border starts a new segment (via startSeg).
> 
>  * lastBEndBorders is the most recent block-end border processed at the
> block-end edge of some cell in each column.  These are only stored to
> determine corner ownership in order to (in turn) determine whether the next
> inline-side border starts a new segment.  (It seems like it stores an extra
> entry at the end of the array that's never used !?)
> 
>  * lastBStartBorder and lastBEndBorder are used to track whether block-side
> borders need to start a new segment (via startSeg).
> 
> Are these statements correct? 

I think, yes. and the namings are not easy to understand it at first.

    
    

    
  


  
thanks for putting the better comments.

For the reftest,

(1)bug1405929.html: I try to make a test which fails without this patch. 
(2)bug1405929-2.html: This is a reftest which I jsut make sure that we draw the BEnd borders together. (same behavior without this patch)


add reftests on Try:
pass with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aa9e96ab68459bc4158e4f7a2b20967b64b46e9&selectedJob=148677107

fails without this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08548c965982e65b39f093eef3fe26f13e90acdf&selectedJob=148678621

        Attachment #8933173 -
        Attachment is obsolete: true

        Attachment #8933536 -
        Flags: review?(dbaron)

    
    

    
  
Note: I have filed Bug 1422238 to consider to add some comments or rename some variables(particularly |lastBEndBorders|, |lastBStartBorder|, |lastBEndBorder|, |lastBlockDirBorders|) in nsTableFrame::CalcBCBorders.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        reftest
        — 
          — Splinter Review
      

    


  

        Attachment #8933181 -
        Attachment is obsolete: true

        Attachment #8934390 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6e4bd1d151
Make sure that we join two BEnd borders when they are on the same rowIndex. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/72958e6e0e4a
reftest for border collapsed table. r=dbaron
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6e6e4bd1d151
https://hg.mozilla.org/mozilla-central/rev/72958e6e0e4a
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox59:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: