Closed Bug 1421887 Opened 7 years ago Closed 7 years ago

[BC] |ResetBStartStart| might introduce weird borders if we don't get the row index right.

Categories

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

Product:
Core
Component:
Layout: Tables
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ywu, Assigned: ywu)

References

(Depends on 1 open bug,
URL
)

Details

Attachments

(3 files, 4 obsolete files)

wrong borders
1.74 KB, text/html
Details
0001-Bug-1421887-fix-the-row-index-to-get-the-right-cell..patch
4.50 KB, patch
ywu
: review+
Details | Diff | Splinter Review
0002-Bug-1421887-add-reftest.-r-dbaron.patch
4.12 KB, patch
ywu
: review+
Details | Diff | Splinter Review
Attached file wrong borders —
|ResetBStartStart| might introduce weird borders
See Also: → 1405929
Follow bug 1405929 comment 45 to further address the comments.
still have to make reftest.
Attachment #8933197 - Attachment is obsolete: true
Attachment #8939715 - Flags: review?(dbaron)
Comment on attachment 8939715 [details] [diff] [review] 0001-Bug-1421887-fix-the-row-index-to-get-the-right-cell..patch >+// XXX eLogicalSideBStart, eLogicalSideIEnd and eLogicalSideIStart >+// have never been called. Consider to use them or remove them. Two comments here: The first is about the English. Rather than "Consider to use them or remove them." it should be "Consider using them or removing them." (Also, "have never been called" should really be "are not used".) The second is about the meaning. I don't think suggesting using them makes sense; we shouldn't add uses of a function just because it's unused. So I think the only reasonable suggestion is not use them. I'd suggest rewording to say something like: // FIXME: The only value callers pass for aSide is eLogicalSideBEnd. Consider removing support for the other three values. r=dbaron with that or similar
Attachment #8939715 - Flags: review?(dbaron) → review+
Comment on attachment 8939716 [details] [diff] [review] 0002-Bug-1421887-add-reftest.-r-dbaron.patch In the future, for cases like this, it might be better to make the reftest a little bit more different from the test, so that its rendering is more obviously correct, rather than using such similar tests. However, I'm struggling to come up with a good definition of what "cases like this" are -- in many cases, what you did here is the right approach, but I think it's probably a little riskier here due to the risk of both test and reference exercising a buggy codepath. But I'd need to think more about in what cases I think that's good advice, since it's certainly not true in general. Anyway, r=dbaron, since this is fine.
Attachment #8939716 - Flags: review?(dbaron) → review+
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #7) > Comment on attachment 8939716 [details] [diff] [review] > 0002-Bug-1421887-add-reftest.-r-dbaron.patch > > In the future, for cases like this, it might be better to make the reftest a > little bit more different from the test, so that its rendering is more > obviously correct, rather than using such similar tests. However, I'm > struggling to come up with a good definition of what "cases like this" are > -- in many cases, what you did here is the right approach, but I think it's > probably a little riskier here due to the risk of both test and reference > exercising a buggy codepath. But I'd need to think more about in what cases > I think that's good advice, since it's certainly not true in general. I see. thank you.
Attachment #8939716 - Attachment is obsolete: true
Attachment #8940072 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: