Add font-size to root <svg> to support proper rem sizing #75

kevinoid · 2014-09-26T16:43:15Z

I noticed an issue that the element and font sizing in the rasterized version of some pages is oddly different from the displayed page. After a bit of investigation, I found that the size of elements with properties specified in CSS3 rem units is determined based on the font-size of the root element which, when appearing in an SVG document, this is the root <svg> element rather than the <html> element which appears inside <foreignObject>.

To support such pages, this pull request attempts to copy the font-size from the HTML document root to the SVG document root using getComputedStyle with fallback to currentStyle and style if neither of the others work (to support detached documents, particularly for testing).

I'd appreciate your thoughts on it. Thanks,
Kevin

The size of elements with properties specified in [CSS3 rem units] [1] is determined based on the font-size of the root element. When appearing in an SVG document, this is the root <svg> element rather than the <html> element which appears inside <foreignObject>. In order to properly size such elements, copy the font-size from the HTML document root to the SVG document root. 1. http://www.w3.org/TR/css3-values/#rem-unit Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

cburgmer · 2014-09-28T12:35:28Z

Hi, thanks for this nice pull request! Looks like we need yet another "hack" for proper rendering.

Your fix looks clean and fixing the rem size calculation through changing the svg element make sense to me. I'll happily merge this into the code base.

In the past I have made sure to base most of the features and fixes on handful of integration tests. So I've made sure to extend the unit tests you have provided and added rem sizing to the current integration test setup (my commit above). So the integration tests in the future will automatically include a test for the rem behaviour. Unit tests have helped me a great deal to identify areas of breakage, whereas the integration tests make sure that it actually works across all the supported browsers. (To run the test, open test/SpecRunner.html in Firefox and test/manualIntegrationTestForWebkit.html for the other browsers.)

However it now seems that the proposed fix isn't enough to solve the bug for this test setup of mine. The font-size isn't correctly read here through window.getComputedStyle(). It looks like this is an issue with the document not being actually rendered. And actually, when I move the font-size reading code towards browser.calculateDocumentContentSize() (which operates the document on an actual iframe) the 10px from the test are correctly read.

Can you reproduce that?

On a side note, there's no need to support IE for now, as <foreignObject> isn't supported at all, making the library currently unfit for IE. Also, the fallback code might not be needed anymore once the above issue is fixed.

kevinoid · 2014-10-02T00:23:56Z

Hi @cburgmer,

Sorry for the slow response. Adding the integration tests is a great idea! I can confirm the getComputedStyle behavior. It looks like Firefox doesn't consider cascaded styles for documents loaded in this way until a layout occurs. Unfortunately it does apply browser defaults (fontSize is returned as "16px" on my machine) so the fallback code I have for this sort of behavior in Chrome won't be hit and adding style="font-size: 10px" to the <html> element isn't a viable workaround.

For the tests it can be worked around by loading the document via an iframe rather than XHR (or rendering it into an iframe after fetching). That solution is not ideal, since it only fixes the tests and it seems likely the issue will affect users as well. However, unconditionally adding the document to an iframe to get this style information is a heavy price for avoiding an issue that won't affect most users. (I know it's already being done in browser.calculateDocumentContentSize, but I'd actually like to see if this can be avoided when the user passes in the size for the same performance reasons.)

I'll look around a bit more to see if there is a way to either cause browsers to calculate the style values for detached documents, or to detect whether style information is missing (or potentially missing) so we can only incur the iframe overhead only when it is strictly necessary.

kevinoid · 2014-10-02T15:54:34Z

Detecting whether layout is required for WebKit browsers is relatively straightforward. Layout is required whenever either null or a CSSStyleDeclaration with all empty-string values is returned from getComputedStyle.

For Gecko browsers, the best heuristic that I could come up with is to check whether document.defaultView is null. This has a lot of false positives since document.cloneNode(true).defaultView is always null but the computed style information is preserved, so forcing a layout is not necessary.

I've also been unable to find a way to force the browser to resolve the cascaded styles short of putting the document into a window of some sort (an iframe being the easiest choice). The performance of this can be improved a bit by using document.importNode with documentElement.replaceChild rather than document.write with documentElement.outerHTML (which also avoids script execution and resource fetching). But it's still pretty heavy.

Thoughts?

cburgmer · 2014-10-05T09:20:17Z

As far as I understand you are concerned with moving more logic into browser.calculateDocumentContentSize() as this adds to the dependency on pre-loading which is a performance issue, one you'd like to improve upon independently.

I agree, that from a performance perspective this is something we'd rather not have. However, there is already quite some functionality that forces layout by the browser:

Canvas size calculation (if you need the minimum necessary canvas, so far mandatory)
optional clipping (still experimental, for calculating the clipped elements bounding box)
optional JavaScript execution (actually in that case currently the document is loaded yet another time).

For my use case much of the bad performance so far came from inlining resources, that's where I've first optimized in the past by adding caching. There's possibly more to be done on the actual execution level, but I obviously haven't put much energy into it.

Looking at the above list I am not too positive we could ever get away with loading the document through an iframe in the browser first. However if there are usecases where none of the features above are needed and good performance is very much required, we could of course aim for that.

As a proposition to split the performance concern off from the feature discussed here: We could first go ahead implementing the feature in such a way that we always layout the document and start another discussion what can be done to improve overall performance. In fact other parts of the library might even expose worse performance - I am not really good at performance measurement.

Regarding your last paragraph about using importNode instead of loading the whole document: This sounds like a neat independent change we could try measure to see if it improves rendering times.

cburgmer · 2014-11-01T09:35:03Z

Is it OK if we start integrating with the proposed solution (load in iframe)? As I said, we could talk about optimizing later on then.

kevinoid · 2014-11-01T18:18:58Z

Sorry for the silence. I've been swamped lately and haven't had a chance to finish reworking this pull request.

I previously worked out some logic for testing whether an iframe is required to get reliable values from getComputedStyle. I threw that in a branch called get-computed-style-forced for reference. But it's clearly the wrong approach, since it could create an iframe for each getComputedStyle call.

I'm fine with a performant implementation now which can be refined later. We could return the font size from calculateDocumentContentSize in addition to the content size (to use the same iframe for both tasks) or add a separate function for calculating the font size which would place the document in an iframe for a second time (if required) to calculate the font size. Any preference?

cburgmer · 2014-11-02T14:17:56Z

I've now merged your change (rebased on the recent changes) and moved the font-size to calculateDocumentContentSize. Let me know if this fits your requirements.

As I indicated earlier, I don't think we need the IE fallback, as sadly IE11 isn't even supported yet.

As for the document loading process: I feel we still don't understand the "lifecycle" of the document object. If I parse HTML to a document nothing is laid-out yet, however if I have a 'attached' object everything works out well. There should be some broader view on how those objects should behave. I feel if we clearly understand what is going on there, we might be able to come up with a clean solution for the font-size calculation.

cburgmer · 2014-11-02T15:01:23Z

http://dev.w3.org/csswg/cssom/#extensions-to-the-window-interface states that getComputedStyle() works on documents attached to a window.

So for one, drawDocument(doc) could check whether doc.defaultView is set. If not, then it needs to be attached first (= rendered in iframe) before any rendering related properties can be read.

If there are valid cases where some of the internal rendering magic is not needed and this could spare us the iframe load, then this could possibly hidden behind a flag.

kevinoid · 2014-11-02T15:27:54Z

Thanks for getting that merged! That looks good and works for me.

Using doc.defaultView works, but it is overly conservative. For example, a node which is cloned still returns correct values for getComputedStyle (at least on Firefox) but doesn't have a defaultView. I would also expect this case to be somewhat common, since it's how I'm currently using rasterizeHTML (clone the document then remove elements I don't want rendered then render). Which is why I check the ownerDocument in addition to the defaultView in getComputedStyleForced.

cburgmer · 2014-11-02T16:58:38Z

I've created a new issue to track this independently. Maybe we can keep adding findings there.

I probably have a bunch of other priorities for the next months, so please don't expect too much from me. Feel free to continue on the PRs :)

Should we close the ticket here?

kevinoid · 2014-11-02T17:06:50Z

Sounds great. I agree that the issue raised in this PR is now solved. We can follow-up on the performance work in #79.

cburgmer added a commit that referenced this pull request Sep 28, 2014

Extend integration tests to include check against #75

c670783

kevinoid pushed a commit to kevinoid/rasterizeHTML.js that referenced this pull request Nov 1, 2014

Extend integration tests to include check against cburgmer#75

c709154

cburgmer added a commit that referenced this pull request Nov 2, 2014

Extend integration tests to include check against #75

7201b24

cburgmer added a commit that referenced this pull request Nov 2, 2014

Read root font-size when document has been rendered. #75

7d89ec2

cburgmer mentioned this pull request Nov 2, 2014

Don't unnecessarily load document in iframe #79

Open

kevinoid closed this Nov 2, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add font-size to root <svg> to support proper rem sizing #75

Add font-size to root <svg> to support proper rem sizing #75

kevinoid commented Sep 26, 2014

cburgmer commented Sep 28, 2014

kevinoid commented Oct 2, 2014

kevinoid commented Oct 2, 2014

cburgmer commented Oct 5, 2014

cburgmer commented Nov 1, 2014

kevinoid commented Nov 1, 2014

cburgmer commented Nov 2, 2014

cburgmer commented Nov 2, 2014

kevinoid commented Nov 2, 2014

cburgmer commented Nov 2, 2014

kevinoid commented Nov 2, 2014

Add font-size to root <svg> to support proper rem sizing #75

Add font-size to root <svg> to support proper rem sizing #75

Conversation

kevinoid commented Sep 26, 2014

cburgmer commented Sep 28, 2014

kevinoid commented Oct 2, 2014

kevinoid commented Oct 2, 2014

cburgmer commented Oct 5, 2014

cburgmer commented Nov 1, 2014

kevinoid commented Nov 1, 2014

cburgmer commented Nov 2, 2014

cburgmer commented Nov 2, 2014

kevinoid commented Nov 2, 2014

cburgmer commented Nov 2, 2014

kevinoid commented Nov 2, 2014