Closed Bug 975935 Opened 6 years ago Closed 6 years ago

rowlines/columnlines broken in Gecko 29+

Categories

(Core :: MathML, defect, P5)

29 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: jkitch, Assigned: jkitch)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files, 2 obsolete files)

Attached file testcase
The attached testcase sets the rowlines and columnlines attributes for an mtable.

In Beta (28.0) the lines appear as expected.

In Aurora (29.0a2) and Nightly (30.0a1) no lines appear (other than the exterior border) and the table is indistinguishable from one without the rowlines/columnlines attributes.
Keywords: regression
Keywords: testcase
OS: Windows 7 → All
Priority: -- → P5
Hardware: x86_64 → All
Version: unspecified → 29 Branch
Possible regression window.

Dec-13 Nightly: Mostly working (although the lines may disappear when reloading).
Dec-14 Nightly: Not working.

Bug 731667 perhaps?
Blocks: 731667
It appears that nsDisplaymtdBorder::Paint() is never actually called.  The class however is created when nsMathMLmtdFrame::ProcessBorders() is called.

It is possible that it is somehow omitted from the list of items sent to FrameLayerBuilder::PaintItems(), as I have been unable to detect any item with class nsDisplaymtdBorder when run under a debugger.  (It isn't hiding under a nsDisplayBorder classname as I commented out the instances where the parent class is created prior to testing).
Assignee: nobody → jkitch.bug
The problem is that when the code checks for the visibility of the border, it checks a data structure that hasn't been updated to include the <mtd> border.  As it isn't visible the border drawing is skipped.

This code overrides GetBounds to take into account the newly added <mtd> border.  (As well as some minor refactoring to nsDisplayBorder to reduce code duplication).
Attachment #8383470 - Flags: review?(roc)
Attachment #8383470 - Flags: review?(fred.wang)
Comment on attachment 8383470 [details] [diff] [review]
Part 1: nsDisplay(mtd)Border changes

Review of attachment 8383470 [details] [diff] [review]:
-----------------------------------------------------------------

BTW it looks to me like nsMathMLmtdFrame needs a Reflow method that sets its overflow areas to include this extra border.

::: layout/base/nsDisplayList.cpp
@@ +2671,5 @@
>  nsDisplayBorder::GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap)
>  {
>    *aSnap = true;
> +  const nsStyleBorder styleBorder = *mFrame->StyleBorder();
> +  return CalculateBounds(styleBorder);

You're making an unnecessary copy here. Just pass *mFrame->StyleBorder() directly
Attachment #8383470 - Flags: review?(roc) → review+
Attached patch Part 2: Tests (obsolete) — Splinter Review
Tests to ensure the regression doesn't happen again.

These tests have been imported from MathJax, with minor modifications to adapt it to our setup.

The tests ensure that the attributes actually do something, that solid lines are solid and that dashed lines are dashed.

Apache 2.0 licensed.
Attachment #8383472 - Flags: review?(fred.wang)
Attachment #8383470 - Flags: review?(fred.wang)
Comment on attachment 8383472 [details] [diff] [review]
Part 2: Tests

Review of attachment 8383472 [details] [diff] [review]:
-----------------------------------------------------------------

I think the comment should be "more than 1px" rather than "at least 1px". I'm a bit concerned that the reftest will fail because of antialiasing and stuff, but let's see the result on the try server. Probably for completeness we should have columnlines-3* too.
Attachment #8383472 - Flags: review?(fred.wang) → review+
Now includes a change to make reflow aware of the additional border.
Attachment #8383470 - Attachment is obsolete: true
Attached patch Part 2: TestsSplinter Review
The additional tests have been added.

Green try run
https://tbpl.mozilla.org/?tree=Try&rev=1a249583cda1
Attachment #8383472 - Attachment is obsolete: true
Attachment #8384105 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a3985ae9699a
https://hg.mozilla.org/mozilla-central/rev/a3ac147bff3f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 731667 regression
User impact if declined: rowlines and columnlines attributes on MathML mtables will stop working
Testing completed (on m-c, etc.): Landed on m-c a few days ago without issue, verified the latest nightly works appropriately.  Patch also includes comprehensive tests.
Risk to taking this patch (and alternatives if risky): Low - impact limited to the broken attributes.
String or IDL/UUID changes made by this patch: None
Attachment #8385988 - Flags: approval-mozilla-aurora?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a3985ae9699a

I've just realised this landed with the wrong reviewer information (r=bz when it should have been r=roc).  My apologies.
Attachment #8385988 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.