Better default styles computation. #108
Conversation
This ensures that browser-stylesheets that have parent tag based rules work correctly. This is evident when doing (for example) a bare `TD` before, the default style was defaulted to be `padding: opx`, but when the `TD` is actually created as a child of the normal hierarchy (e.g. `TABLE`, `TBODY`, `TR`, `TD`) then the defaulted padding is `padding: 1px`. Refactored a bit for clarity Resolves #106 and thus also Resolves #95
|
Hey @meche-gh, this should do the trick... I also refactored that part because it was getting a bit long-winded. |
|
That's a fair concern... I ran some tests against some huge tests I have
and found 5 tag depth is pretty much the max before hitting a stopper since
we're not factoring siblings in (and siblings are a cache hit). I suspect
it's not a huge issue, but we would need at least 4 for the fix to kick in
for `TD` cells.
I wonder if we have a reproduction for the #70 issue handy?
Also wondering if we would be better served for now in pulling in your post
filter stuff and maybe putting the optimizations behind a feature flag?
|
|
For |
|
That may work... we're not going to save anything walking up the parentage,
and we need to create the tags we found. So this would effectively save
caching a few edge cases (and smaller tagKey strings...but that is a tiny
factor). I will test with a bunch of big pages and see what an instrumented
run shows.
|
I was thinking the filter could be used for the other dom-to-image fork packages. At the same time, I'd imagine a lot of opt-ins. I'll see if I can refactor my secret WIP branch into a neat minimal PR. |
|
Just to be clear, we do want to be creating default elements against the full hierarchy tree, and my concern about the perf can be handled by a change to the logic for creating the cache key. I.e. |
src/dom-to-image-more.js
Outdated
| 'SVG', | ||
| 'TABLE', | ||
| 'UL', | ||
| // these are ultimate stoppers in case something drastic changes in how the DOM works | ||
| 'BODY', | ||
| 'HEAD', | ||
| 'HTML', |
There was a problem hiding this comment.
[nit] The svg addition is ultimately custom - it's a performance boost!
SVG tags inside an <svg> are not going to have their default CSS change because of the parent elements outside of their <svg> tag
There was a problem hiding this comment.
You can also add <math> as the MathML sectioning root. It is not part of the HTML spec unfortunately - it has the same relation to HTML that JS does!
There was a problem hiding this comment.
Little gotcha about math-the tagName property is lowercase. I personally think we benefit from lowercasing the key and the list of tags (to handle case-sensitive tag names).
There was a problem hiding this comment.
math added, in lower case... I suspect when browsers actually do adopt the tag it'll morph to uppercase...
There was a problem hiding this comment.
Cool. We need to put svg (another lowercase exception) after the "these are ultimate stoppers" comment.
Just to be sure it doesn't get deleted later when the list gets updated
There was a problem hiding this comment.
- We need to lowercase
svgalso! - The
svgtag won't appear in the MDN list so we should move it under the// these are ultimate stopperscomment
There was a problem hiding this comment.
Oh, doh... Fixed that real quick... completely missed what you were going for...
| return defaultStyle; | ||
|
|
||
| function computeTagHierarchy(sourceNode) { | ||
| const ELEMENT_NODE = 1; |
There was a problem hiding this comment.
| const ELEMENT_NODE = 1; | |
| const ELEMENT_NODE = Node.ELEMENT_NODE || 1; |
Need to be "playing it safe" with this global also!
There was a problem hiding this comment.
Pulled those changes in and now have the ability to key the defaultStyleCache with either the fill tag hierachy (strict mode) or just ascentStopper + tagName ('relaxed` mode). This should make it possible revert if we broke something.
Also cleaned up the Promise handling to return the right things along the chain.
This necessitates passing the entire `options` block down the `cloneNode` path, that will be handy later anyway. Moved `SVG` tag to where it makes sense.
Uses the correct tag hierarchy when computing the default style to account for (implicit) browser stylesheets that use tags within a hierarchy to specify default styles.
We create the elements in the sandbox with all the required parent nodes to match up to the first block-level element, compute the default styling and then tear down that hierarchy. This work is still only done once per tag-and-parents combination.
Resolves #106 and thus also Resolves #95