Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getContainer() delivers wrong element if element has size defined in percent #50

Closed
d0cm0d opened this issue Feb 5, 2018 · 8 comments
Closed
Labels

Comments

@d0cm0d
Copy link

d0cm0d commented Feb 5, 2018

Hi there,
first of all: great project! Thanks!

Now my issue/question:
If the parent element has a width defined in percent, the getContainer() function returns a wrong element. If I am right, it should return the element itself, because the width has a fixed definition (isIntrinsicSize() returns false). So there should be the following check after the isFixedSize check:

            else if (!isIntrinsicSize(element, prop)) {
                cache[prop] = element;
            }

Or am I wrong and the check is missing with a reason, and if yes, what reason?
Thanks, best regards, Pascal

@ausi
Copy link
Owner

ausi commented Feb 5, 2018

If I am right, it should return the element itself, because the width has a fixed definition

If the element has a percentage width, the dimension is not fixed in all cases. In the following example, the percentage width is not fixed:

<div style="width: 1000px;">
	<div style="float: left;">
		<div style="width: 50%;">
			<div>My Element</div>
		</div>
	</div>
</div>

But with a small change (removing the float from the parent) it becomes a fixed width and thus a valid container:

<div style="width: 1000px;">
	<div style="float: none;">
		<div style="width: 50%;">
			<div>My Element</div>
		</div>
	</div>
</div>

So in the first case the outermost <div> should get returned by getContainer() and in the second case the <div> with the percentage width should get returned.

Does this explanation help? Or do you have an example code where it doesn’t work as intended?

@ausi ausi added the question label Feb 5, 2018
@d0cm0d
Copy link
Author

d0cm0d commented Feb 6, 2018

Hallo,
thank you for your comment. It made it more clear. Nevertheless, that is not the problem.
I was digging a little bit deeper and recognized, that the problem comes from the stylesheet parsing.
In my css, I include other css files. In this case, the buildStyleCacheFromRules() function fails, because the "if (rules[i].type === 1)" is false (rules[i].type is 3) and the "else if (rules[i].cssRules)" is undefined.
In this case (rules[i].type === 3), the css rules are in the object rules[i].styleSheet.cssRules.
So if we add the following case to the if, it should work:
else if (rules[i].type === 3 && rules[i].styleSheet.cssRules) { buildStyleCacheFromRules(rules[i].styleSheet.cssRules); }
I've just tested it and it works.
And I created a codepen to showcase the problem: https://codepen.io/anon/pen/wyWbWm

What do you think?

@ausi
Copy link
Owner

ausi commented Feb 6, 2018

This sounds like the issue #49.
This was fixed in 579d039 in the development branch and will be fixed with the next release.

@d0cm0d
Copy link
Author

d0cm0d commented Feb 6, 2018

Yes, it's the same issue. I've fixed it a little bit different, because imported stylesheets can contain imported stylesheets again. I've just created a pull request, that - at least I think so - handles all cases and is very simple.
Thank you, best regards, Pascal

@ausi
Copy link
Owner

ausi commented Feb 6, 2018

Did you test the changes from 579d039 ?
Nested imports should work there too.

@ausi
Copy link
Owner

ausi commented Feb 6, 2018

Ahh, sorry. I missed something. You are talking about the buildStyleCacheFromRules() function, not about parseRules().

You are right, that needs to be fixed too. I close this issue in favor of your pull request #51.

@ausi ausi closed this as completed Feb 6, 2018
@d0cm0d
Copy link
Author

d0cm0d commented Feb 7, 2018

Hello,
I just recognized a problem with my fix with Firefox. Firefox has a bug that restricts js access to css imported with @import ("SecurityError: The operation is insecure error").
I just updated my fix and will start a new pull request.

@ausi
Copy link
Owner

ausi commented Feb 7, 2018

Great. But you can push more commits to your existing Fix-@import-stylesheet-parsing branch. This way you don’t have to create a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants