| Summary: | Changes in the stretchy attribute do not update rendering | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alejandro G. Castro <alex> | ||||||||||||
| Component: | MathML | Assignee: | Alejandro G. Castro <alex> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | cfleizach, commit-queue, darin, dbarton, esprehn+autocc, fred.wang, glenn, kondapallykalyan, mrobinson | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 238244 [details]
Patch
Comment on attachment 238244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238244&action=review > Source/WebCore/mathml/MathMLElement.cpp:302 > + if (renderer() && (name == MathMLNames::stretchyAttr) && !equalIgnoringCase(oldValue, newValue)) > + renderer()->setNeedsLayoutAndPrefWidthsRecalc(); This is a very unusual idiom. Do we have anything like this in any other element in WebKit? Also, it’s surprising to see this in the base MathML element. Is stretchy supported in every single specific MathML element? Comment on attachment 238244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238244&action=review >> Source/WebCore/mathml/MathMLElement.cpp:302 >> + renderer()->setNeedsLayoutAndPrefWidthsRecalc(); > > This is a very unusual idiom. Do we have anything like this in any other element in WebKit? > > Also, it’s surprising to see this in the base MathML element. Is stretchy supported in every single specific MathML element? We also would want to trigger the work based on a change in the parsed value, not based on a change in the string. For example, there are many ways to specify an invalid value and they should all be considered equal, not distinct. (In reply to comment #2) > (From update of attachment 238244 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238244&action=review > > > Source/WebCore/mathml/MathMLElement.cpp:302 > > + if (renderer() && (name == MathMLNames::stretchyAttr) && !equalIgnoringCase(oldValue, newValue)) > > + renderer()->setNeedsLayoutAndPrefWidthsRecalc(); > > This is a very unusual idiom. Do we have anything like this in any other element in WebKit? > It is unusual, you are right, you just can find in HTMLTextAreaElement parseAttribute a call to setNeedsLayoutAndPrefWidthsRecalc when rowsAttr changes. I guess it means an attribute that modifies something visual that is not represented using a CSS property, I've seen MathML unfortunately uses a lot of style definition itself. Wrt to use parseAttribute or attributeChanged I decided using attributeChanged because we can check old and new value without creating a new attribute in the object. But your next comment explains clearly why a parsed and stored value is the right option. > Also, it’s surprising to see this in the base MathML element. Is stretchy supported in every single specific MathML element? It is just the mo tag, operator, you are right, when I checked the tags file I missed the MathMLTextElement which is the one representing it. Thanks for the review, I'll modify and update accordingly. (In reply to comment #3) > We also would want to trigger the work based on a change in the parsed value, not based on a change in the string. For example, there are many ways to specify an invalid value and they should all be considered equal, not distinct. Very good point, I think storing the parsed value in an attribute using parseAttribute and checking the change of that value to request the relayout would be a better option. Thanks again for the advise. Comment on attachment 238244 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238244&action=review >>>> Source/WebCore/mathml/MathMLElement.cpp:302 >>>> + renderer()->setNeedsLayoutAndPrefWidthsRecalc(); >>> >>> This is a very unusual idiom. Do we have anything like this in any other element in WebKit? >>> >>> Also, it’s surprising to see this in the base MathML element. Is stretchy supported in every single specific MathML element? >> >> We also would want to trigger the work based on a change in the parsed value, not based on a change in the string. For example, there are many ways to specify an invalid value and they should all be considered equal, not distinct. > > It is unusual, you are right, you just can find in HTMLTextAreaElement parseAttribute a call to setNeedsLayoutAndPrefWidthsRecalc when rowsAttr changes. I guess it means an attribute that modifies something visual that is not represented using a CSS property, I've seen MathML unfortunately uses a lot of style definition itself. > > Wrt to use parseAttribute or attributeChanged I decided using attributeChanged because we can check old and new value without creating a new attribute in the object. But your next comment explains clearly why a parsed and stored value is the right option. Somehow we need to share the logic appropriately with RenderMathMLOperator, which is the code that actually parses the stretch attribute. Created attachment 238845 [details]
Patch
(In reply to comment #6) > Somehow we need to share the logic appropriately with RenderMathMLOperator, which is the code that actually parses the stretch attribute. Right, just implemented it using the parsing from the RenderMathMLOperator with this last patch. Thanks for the comments. Comment on attachment 238845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238845&action=review Looks good. review- because of the bad cast > Source/WebCore/mathml/MathMLTextElement.cpp:69 > + if ((name == stretchyAttr) && renderer()) Don’t need the parentheses here. This is a common enough idiom and we don’t use the parentheses everywhere else. I understand that sometimes parentheses make operator precedence clearer, but == vs. && shows up so often that it doesn’t require the extra emphasis. > Source/WebCore/mathml/MathMLTextElement.cpp:70 > + toRenderMathMLOperator(renderer())->setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Stretchy, value, true); What guarantees the renderer is a RenderMathMLOperator? The createElementRenderer function creates objects of many different types; if this isn’t a RenderMathMLOperator then this will be a bad cast. > Source/WebCore/mathml/MathMLTextElement.cpp:72 > + else > + MathMLElement::parseAttribute(name, value); I don’t think this needs to be an else. I think we should call through to the base class parseAttribute even if we do the setOperatorFlagFromAttributeValue work. > Source/WebCore/mathml/MathMLTextElement.h:43 > +protected: > + virtual void parseAttribute(const QualifiedName&, const AtomicString&) override; Why protected instead of private? I don’t see any classes that derive from this one. I also suggest we mark this class final for the same reason. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1181 > + const AtomicString& attributeValue = element().fastGetAttribute(name); > + > + setOperatorFlagFromAttributeValue(flag, attributeValue); This code would read better without the local variable. > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:81 > + void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag, const AtomicString&, bool doLayoutIfNeeded = false); I think we might need the name "attributeValue" or "value" for the AtomicString argument. It’s not entirely clear what that argument is from the type alone. We try to avoid using booleans for these kinds of arguments where you pass a constant, because “true” is not clear at the call site. Instead we normally use enums or two separate functions for something like this. In this case, I think we should have two separate functions, an inner one that does not do setNeedsLayoutAndPrefWidthsRecalc and an outer one that does. I’m not sure that the right name for that is “layout”. It’s easy for the outer function to get the operator flags before and after; it can just call the inner function. Thanks for the comments :). I'll review accordingly, just some comments inlined to explain some of the rationale of the original patch. (In reply to comment #9) > > Source/WebCore/mathml/MathMLTextElement.cpp:70 > > + toRenderMathMLOperator(renderer())->setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Stretchy, value, true); > > What guarantees the renderer is a RenderMathMLOperator? The createElementRenderer function creates objects of many different types; if this isn’t a RenderMathMLOperator then this will be a bad cast. > I was thinking about adding the condition but assumed if we are parsing the stretchy attribute we are checking a RenderMathMLOperator, I mean if (name == stretchyAttr) is true, and because operator is the only one that accepts stretchyAttr this is a RenderMathMLOperator. It is true this is my supposition about how the parseAttribute is being called. I'll add the condition because it is even clearer for the code. > > Source/WebCore/mathml/MathMLTextElement.cpp:72 > > + else > > + MathMLElement::parseAttribute(name, value); > > I don’t think this needs to be an else. I think we should call through to the base class parseAttribute even if we do the setOperatorFlagFromAttributeValue work. > My understanding to use the else was that no other class is parsing this attribute so we can avoid the call in that case. > > Source/WebCore/mathml/MathMLTextElement.h:43 > > +protected: > > + virtual void parseAttribute(const QualifiedName&, const AtomicString&) override; > > Why protected instead of private? I don’t see any classes that derive from this one. I also suggest we mark this class final for the same reason. > No good reason for this, I'll do it. > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1181 > > + const AtomicString& attributeValue = element().fastGetAttribute(name); > > + > > + setOperatorFlagFromAttributeValue(flag, attributeValue); > > This code would read better without the local variable. > I agree. > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:81 > > + void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag, const AtomicString&, bool doLayoutIfNeeded = false); > > I think we might need the name "attributeValue" or "value" for the AtomicString argument. It’s not entirely clear what that argument is from the type alone. > > We try to avoid using booleans for these kinds of arguments where you pass a constant, because “true” is not clear at the call site. Instead we normally use enums or two separate functions for something like this. In this case, I think we should have two separate functions, an inner one that does not do setNeedsLayoutAndPrefWidthsRecalc and an outer one that does. I’m not sure that the right name for that is “layout”. It’s easy for the outer function to get the operator flags before and after; it can just call the inner function. It is interesting because I was trying both options, with two functions or with the attribute, this seemed to me a better approach even without the enum but it is true both were quite similar. I'll reimplement with the two functions now that you point out that solution. Thanks again for the detailed comments :). Created attachment 238919 [details]
Patch
I've added the proposed modifications.
(In reply to comment #10) > I was thinking about adding the condition but assumed if we are parsing the stretchy attribute we are checking a RenderMathMLOperator, I mean if (name == stretchyAttr) is true, and because operator is the only one that accepts stretchyAttr this is a RenderMathMLOperator. It is true this is my supposition about how the parseAttribute is being called. I'll add the condition because it is even clearer for the code. A side note: You don’t have to guess about this. You can test by creating the element which would use a different kind of renderer and adding a stretchy attribute (which should be ignored). Then you will see that this code gets called in cases where this would be a bad cast, or provide that it doesn’t. > > > Source/WebCore/mathml/MathMLTextElement.cpp:72 > > > + else > > > + MathMLElement::parseAttribute(name, value); > > > > I don’t think this needs to be an else. I think we should call through to the base class parseAttribute even if we do the setOperatorFlagFromAttributeValue work. > > > > My understanding to use the else was that no other class is parsing this attribute so we can avoid the call in that case. We are inconsistent about this. Some classes call through to the base class even though the base class does nothing. HTMLElement::parseAttribute is an example of this. In other cases, the derived classes don’t call through because they know the base class won’t do anything with a particular attribute. The best argument for calling through is robustness as we change what attributes are handled over time. The best argument for not calling through is slightly better performance by skipping unneeded code. Given that, we should not call through to the base class for stretchyAttr unconditionally, regardless of the state of the renderer. Comment on attachment 238919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238919&action=review I’m going to say review+ although it’s not good to have a bug fix here (case sensitivity on true and false) with no test coverage. > LayoutTests/mathml/presentation/mo-stretch-update.html:14 > + if (window.testRunner) { > + testRunner.waitUntilDone(); > + } No need for braces here. > LayoutTests/mathml/presentation/mo-stretch-update.html:19 > + window.setTimeout(function() No need for "window." here. > LayoutTests/mathml/presentation/mo-stretch-update.html:26 > + window.addEventListener('load', updatePageAfterRendering, false); No need for "window." here. > Source/WebCore/mathml/MathMLTextElement.cpp:70 > + if (name == stretchyAttr && renderer() && renderer()->isRenderMathMLOperator()) > + toRenderMathMLOperator(renderer())->setOperatorFlagAndScheduleLayoutIfNeeded(MathMLOperatorDictionary::Stretchy, value); Given our conversation about not calling through, this could also be: if (name == stretchyAttr) { if (renderer() && renderer()->isRenderMathMLOperator()) toRenderMathMLOperator(renderer())->setOperatorFlagAndScheduleLayoutIfNeeded(MathMLOperatorDictionary::Stretchy, value); return; } But it’s also fine the way you have it. > Source/WebCore/mathml/MathMLTextElement.h:49 > + virtual void parseAttribute(const QualifiedName&, const AtomicString&) final; This needs to be marked override. This *class* should be marked final. Then we would not bother marking this particular function final, since all functions would be final. > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1179 > + if (equalIgnoringCase(attributeValue, "true")) Good bug fix. But no test coverage for this! > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1181 > + else if (equalIgnoringCase(attributeValue, "false")) Good bug fix. But no test coverage for this! > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:83 > protected: Almost everything below this point should be private rather than protected. Things can be protected if they need to be used in RenderMathMLRadicalOperator, but as much as possible should be private instead for better understanding of the class and easier refactoring later. > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:188 > void setOperatorFlagFromAttribute(MathMLOperatorDictionary::Flag, const QualifiedName&); As with most everything above, this should be private rather than protected. Also, function members should typically appear before data members in the class definition, which typically are listed all at the end. It’s peculiar to have this paragraph of four functions at the bottom after the data members. Not caused by the new patch, but could be better. > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:189 > + void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag, const AtomicString& attributeValue); This new function should be private rather than protected. > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:191 > virtual void SetOperatorProperties(); Incorrect capitalization. Not related to this patch, but not right either. > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1179 > > + if (equalIgnoringCase(attributeValue, "true")) > > Good bug fix. But no test coverage for this! > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1181 > > + else if (equalIgnoringCase(attributeValue, "false")) > > Good bug fix. But no test coverage for this! > mmmh, AFAIK the attributes in MathML are case-sensitive. From http://www.w3.org/TR/MathML3/chapter2.html, "Note that the color name keywords are not case-sensitive, unlike most keywords in MathML attribute values, for compatibility with CSS and HTML.". Check the RelaxNG schema to be sure. Thanks for the review, I'll review and commit with the changes pointed out. Regarding the case sensitiveness I'll check Fred comments with the RelaxNG file, and change accordingly. I'll create a the test case in a different bug to check that for the future. (In reply to comment #14) > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1179 > > > + if (equalIgnoringCase(attributeValue, "true")) > > > > Good bug fix. But no test coverage for this! > > > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1181 > > > + else if (equalIgnoringCase(attributeValue, "false")) > > > > Good bug fix. But no test coverage for this! > > > > mmmh, AFAIK the attributes in MathML are case-sensitive. From http://www.w3.org/TR/MathML3/chapter2.html, "Note that the color name keywords are not case-sensitive, unlike most keywords in MathML attribute values, for compatibility with CSS and HTML.". Check the RelaxNG schema to be sure. You are right, thanks for the comments Fred. The case of the stretchy for instance is: http://www.w3.org/Math/RelaxNG/mathml3/mathml3-presentation.rng ... <attribute name="stretchy"> <choice> <value>true</value> <value>false</value> </choice> </attribute> ... I'll remove that part of the patch and upload a test case for that in a different bug to avoid future problems. (In reply to comment #13) > (From update of attachment 238919 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238919&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:83 > > protected: > > Almost everything below this point should be private rather than protected. Things can be protected if they need to be used in RenderMathMLRadicalOperator, but as much as possible should be private instead for better understanding of the class and easier refactoring later. > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:188 > > void setOperatorFlagFromAttribute(MathMLOperatorDictionary::Flag, const QualifiedName&); > > As with most everything above, this should be private rather than protected. > > Also, function members should typically appear before data members in the class definition, which typically are listed all at the end. It’s peculiar to have this paragraph of four functions at the bottom after the data members. Not caused by the new patch, but could be better. > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:189 > > + void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag, const AtomicString& attributeValue); > > This new function should be private rather than protected. > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:191 > > virtual void SetOperatorProperties(); > > Incorrect capitalization. Not related to this patch, but not right either. I've uploaded a followed-up patch fixing these style problems to the bug #137611. (In reply to comment #16) > > The case of the stretchy for instance is: > > http://www.w3.org/Math/RelaxNG/mathml3/mathml3-presentation.rng > > ... > <attribute name="stretchy"> > <choice> > <value>true</value> > <value>false</value> > </choice> > </attribute> > ... > > I'll remove that part of the patch and upload a test case for that in a different bug to avoid future problems. I've uploaded a test to a followed-up bug #137602. Created attachment 239625 [details]
Patch
Updated the patch with the suggestions and replacing the toRender* with the new dowcast. Too many changes so I'd rather ask for a final rubberstamp.
Comment on attachment 239625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239625&action=review > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:190 > + void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag, const AtomicString& attributeValue); Should be private rather than protected. (In reply to comment #20) > (From update of attachment 239625 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239625&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:190 > > + void setOperatorFlagFromAttributeValue(MathMLOperatorDictionary::Flag, const AtomicString& attributeValue); > > Should be private rather than protected. Yep, when splitting the patch I added this change to the patch in bug #137611, because there is no private section now. Landed http://trac.webkit.org/changeset/174677 Thanks for the help. |
Created attachment 238242 [details] Test showing the problem If uploaded a test to explain when this happend, in the test the operator should be stretched eventually.