Bug 160077

Summary: Painting issues with menclose and reflow / repaint
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, bfulgham, commit-queue, dbarton, dino, distler, esprehn+autocc, glenn, kondapallykalyan, rego, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 161126    
Attachments:
Description Flags
Testcase Dirac Equation
none
Patch none

Frédéric Wang (:fredw)
Reported 2016-07-22 00:20:43 PDT
Created attachment 284311 [details] Testcase Dirac Equation The following issue has been reported by Jacques Distler on his Instiki sandbox page: "Multiple missing symbols in the Dirac Equation.". I'm not exactly sure when it happens, it seems that his page currently uses MathJax for WebKit, which might causing unnecessary reflow or style that make the analysis complicated. I have isolated a testcase, but I'm not sure it is fully reduced (the bug does not show up for a single equation for example). I noticed that changing the CSS font-size or selecting the content causes the 0 or the operators to disappear. It seems that the menclose element is also involved.
Attachments
Testcase Dirac Equation (1.06 KB, text/html)
2016-07-22 00:20 PDT, Frédéric Wang (:fredw) 		no flags
Details
Patch (10.04 KB, patch)
2016-07-22 01:51 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2016-07-22 01:51:39 PDT
Created attachment 284318 [details] Patch
Brent Fulgham
Comment 2 2016-07-22 09:36:06 PDT
Comment on attachment 284318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284318&action=review I am confident your patch fixes the problem you describe, but I think we should try to fix it in GraphicsContextStateSaver, unless we discover that GCSS explicitly avoids protecting the paint rect. > Source/WebCore/ChangeLog:9 > + restored by GraphicsContextStateSaver. To avoid some weird rendering bugs in MathOperator Doesn't it seem like the problem we should be solving is why GraphicsContextStateSaver isn't working properly? > Source/WebCore/ChangeLog:11 > + original PaintInfo before applying the transform. Perhaps there is no performance difference in ensuring that GraphicsContextStateSaver does the right thing, versus your (and SVG's) solution of duplicating the PaintInfo before making adjustments. Still, this seems like a bad pattern, since we presumable knew about the problem when it was fixed in SVG, but did not realize the problem existed here as well. It sounds like we are very likely to have even more cases of this issue in our code!
Brent Fulgham
Comment 3 2016-07-22 09:39:33 PDT
Comment on attachment 284318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284318&action=review >> Source/WebCore/ChangeLog:11 >> + original PaintInfo before applying the transform. > > Perhaps there is no performance difference in ensuring that GraphicsContextStateSaver does the right thing, versus your (and SVG's) solution of duplicating the PaintInfo before making adjustments. Still, this seems like a bad pattern, since we presumable knew about the problem when it was fixed in SVG, but did not realize the problem existed here as well. > > It sounds like we are very likely to have even more cases of this issue in our code! Wait -- I'm way off base here. The GraphicsContextStateSaver only knows about the context, so of course it can't protect the rect. It seems like your solution of copying the PaintInfo and transforming the copy is the right approach, though I do worry that this pattern may not be handled properly in other parts of the code resulting in weird drawing problems.
Brent Fulgham
Comment 4 2016-07-22 09:41:59 PDT
Zalan: The bug Frédéric discovered here seems likely to exist in other parts of our code. Do we have some way to automate the process of protecting the drawing state when doing a transform like this code? Since this was something found and fixed in SVG, and now again in MathML, I'm worried we have other cases where this is being handled improperly.
WebKit Commit Bot
Comment 5 2016-07-23 01:20:58 PDT
Comment on attachment 284318 [details] Patch Clearing flags on attachment: 284318 Committed r203639: <http://trac.webkit.org/changeset/203639>
WebKit Commit Bot
Comment 6 2016-07-23 01:21:02 PDT
All reviewed patches have been landed. Closing bug.
Frédéric Wang (:fredw)
Comment 7 2016-07-23 01:26:30 PDT
I agree with Brent, but does not see an easy way to fix it (unless we write such a save/restore mechanism for the paintInfo too). For the menclose, we could also have gotten rid of the applyTransfrom since that's just a simple translate (that's how I initially fixed this bug). However, I did notsee how to do that for MathOperator as the transform is more complex, so just copied SVG in the two cases for consistency.
Note You need to log in before you can comment on or make changes to this bug.