Closed Bug 745685 Opened 9 years ago Closed 9 years ago
Parse <font size> per HTML5
Our current algorithm for parsing <font size> is something like: 1) Strip whitespace from start and end. 2) If the string begins with "+" or "-": a) Truncate the string at the first non-digit character after the first. b) If the remaining string is exactly one of "-10", "-9", ..., "-0", "+0", "+1", ..., "+10", then add three and clamp to [1, 7]. c) Otherwise, parse error. 3) Otherwise, parse as an integer, dropping any trailing non-digit characters. HTML5 says what to do here: http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#phrasing-content-1 It's almost the same, except that it will accept any integer with a plus or minus sign, not just -10 to +10. So it treats +11 the same as +10. This makes more sense than what Gecko currently does.
While we're at it: is there a reason we normalize during serialization here? This: data:text/html,<!doctype html> <font size="+1x"> <script> document.body.textContent = document.body.firstChild.getAttribute("size") </script> produces "+1x" in Chrome 19 dev and Opera Next 12.00 alpha, but "+1" in IE10 Developer Preview and in Gecko. Normalizing to "+1" doesn't follow any spec that I know of, and seems unnecessarily confusing.
I patterned the new function after nsAttrValue::StringToInteger, but it's different enough that I didn't think code reuse was practical (in particular, different handling of initial + and -).
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #615274 - Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-u all]
Autoland Patchset: Patches: 615274 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=d761b0aa52bd Try run started, revision d761b0aa52bd. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d761b0aa52bd
Try run for d761b0aa52bd is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=d761b0aa52bd Results (out of 219 total builds): exception: 2 success: 186 warnings: 31 Builds (or logs if builds failed) available at: http://firstname.lastname@example.org
Comment on attachment 615274 [details] [diff] [review] Patch v1 Reparsing this on every mapping operation seems like a pretty noticeable performance hit... I would really prefer we did not do that.
Attachment #615274 - Flags: review?(bzbarsky) → review-
Okay, so this patch version stores both the parsed and unparsed values in the nsAttrValue. Really we should be doing this for all attributes. Throwing out the original string means that in general, if you do el.setAttribute(x, y), immediately calling el.getAttribute(x) will not return y, which is certainly a violation of the DOM specs. Is there a reason we do it this way? Are there significant memory savings, given that we're usually using nsIAtom anyway?
Whiteboard: [autoland-try:-u all]
Autoland Patchset: Patches: 616042 Branch: mozilla-central => try Destination: http://hg.mozilla.org/try/pushloghtml?changeset=21ec0c73d987 Try run started, revision 21ec0c73d987. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=21ec0c73d987
In most cases, we can serialize directly from the parsed value; e.g., we don't need to store "10", but we need to (and do) store "10foo".
Hmm, that's true. But we don't store "+1foo" without my patch. Should I try to optimize for the case where the value is exactly "1", "2", "3", "4", "5", "6", or "7"? I could, but would that really save much?
Try run for 21ec0c73d987 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=21ec0c73d987 Results (out of 221 total builds): exception: 1 success: 187 warnings: 33 Builds (or logs if builds failed) available at: http://email@example.com
Comment on attachment 616042 [details] [diff] [review] Patch v2 r=me. Nice.
Attachment #616042 - Flags: review?(bzbarsky) → review+
Target Milestone: --- → mozilla15
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Documented: https://developer.mozilla.org/en/HTML/Element/font#Gecko_notes And listed on Firefox 15 for developers.
You need to log in before you can comment on or make changes to this bug.