Bug 194940 - Fix unitless usage of mathsize
Summary: Fix unitless usage of mathsize
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis (away until 22nd August)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-22 01:38 PST by Rob Buis (away until 22nd August)
Modified: 2019-03-04 01:21 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.97 KB, patch)
2019-02-22 01:40 PST, Rob Buis (away until 22nd August)
no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2019-02-25 00:32 PST, Rob Buis (away until 22nd August)
no flags Details | Formatted Diff | Diff
Patch (1.77 KB, patch)
2019-02-25 02:06 PST, Rob Buis (away until 22nd August)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis (away until 22nd August) 2019-02-22 01:38:22 PST
Computation of unitless values for mathsize is wrong, causing these tests to fail:
* http://www.w3c-test.org/mathml/relations/css-styling/lengths-1.html
* http://www.w3c-test.org/mathml/relations/css-styling/lengths-3.html
Comment 1 Rob Buis (away until 22nd August) 2019-02-22 01:40:31 PST
Created attachment 362708 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2019-02-22 01:51:11 PST
Comment on attachment 362708 [details]
Patch

Great thanks. I guess it's a bit hacky and I think Gecko has similar hacks. Probably nonzero unitless values should not be supported.
Comment 3 WebKit Commit Bot 2019-02-22 02:42:49 PST
Comment on attachment 362708 [details]
Patch

Clearing flags on attachment: 362708

Committed r241942: <https://trac.webkit.org/changeset/241942>
Comment 4 WebKit Commit Bot 2019-02-22 02:42:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2019-02-22 02:43:22 PST
<rdar://problem/48306931>
Comment 6 Darin Adler 2019-02-22 09:58:06 PST
Oh, no! Don’t add more uses of String::format!
Comment 7 Frédéric Wang (:fredw) 2019-02-22 10:06:43 PST
(In reply to Frédéric Wang (:fredw) from comment #2)
> Comment on attachment 362708 [details]
> Patch
> 
> Great thanks. I guess it's a bit hacky and I think Gecko has similar hacks.
> Probably nonzero unitless values should not be supported.

Let's ask the MathML CG to remove these special parsing so that we don't need these hacks anymore:
https://github.com/mathml-refresh/mathml/issues/24
https://github.com/mathml-refresh/mathml/issues/7
Comment 8 Frédéric Wang (:fredw) 2019-02-22 10:54:44 PST
Comment on attachment 362708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362708&action=review

> Source/WebCore/mathml/MathMLElement.cpp:105
> +        return String::format("%.3f%%", unitlessValue * 100.0);

I suspect this is not super reliable. I seems to get rounding errors with my local build.
Comment 9 Rob Buis (away until 22nd August) 2019-02-22 11:35:08 PST
(In reply to Darin Adler from comment #6)
> Oh, no! Don’t add more uses of String::format!

Oops, I was not aware of https://bugs.webkit.org/show_bug.cgi?id=194752! Maybe it could be renamed to deprecatedFormat. I am happy to fix it in a follow up though.
Comment 10 Rob Buis (away until 22nd August) 2019-02-25 00:32:14 PST
Reopening to attach new patch.
Comment 11 Rob Buis (away until 22nd August) 2019-02-25 00:32:16 PST
Created attachment 362888 [details]
Patch
Comment 12 Rob Buis (away until 22nd August) 2019-02-25 00:48:34 PST
Already fixed.
Comment 13 Rob Buis (away until 22nd August) 2019-02-25 02:06:29 PST
Reopening to attach new patch.
Comment 14 Rob Buis (away until 22nd August) 2019-02-25 02:06:31 PST
Created attachment 362890 [details]
Patch
Comment 15 Darin Adler 2019-03-03 00:32:10 PST
Well, I re-landed my patch that fixes it. You are welcome to refine further.
Comment 16 Rob Buis (away until 22nd August) 2019-03-04 01:21:58 PST
Hi Darin,

(In reply to Darin Adler from comment #15)
> Well, I re-landed my patch that fixes it. You are welcome to refine further.

Thanks for the heads up. We are happy with the change so I will close this bug. Note that we are still evaluating usage of unitless values in mathsize for the next version of the  MathML spec, but if we drop support we will likely do it in a new bug.