_AlignmentsMixin.colorForBase doesn't pick up styles loaded via less css preprocessor #527

Closed
yeban opened this Issue Nov 6, 2014 · 6 comments

Comments

Projects
None yet
2 participants
@yeban
Contributor

yeban commented Nov 6, 2014

I traced down the issue to the conditioinal at L117. It looks like stylesheets loaded with less have href=null set on them and thus the if block never gets executed. It doesn't look like the code in if block is modifying stylesheets so it's perhaps safe to remove the conditional?

@yeban yeban changed the title from _ to _AlignmentsMixin.colorForBase doesn't pick up styles loaded via less.js Nov 6, 2014

@yeban yeban changed the title from _AlignmentsMixin.colorForBase doesn't pick up styles loaded via less.js to _AlignmentsMixin.colorForBase doesn't pick up styles loaded via less css preprocessor Nov 6, 2014

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Nov 6, 2014

Contributor

Firefox throws a security exception if it touches a stylesheet from a different domain or server (or in my case, from add-ons including styles from resource://)

I suppose you could try a workaround but I would like to keep the original logic somewhat present to avoid that. Original bug #451

Contributor

cmdcolin commented Nov 6, 2014

Firefox throws a security exception if it touches a stylesheet from a different domain or server (or in my case, from add-ons including styles from resource://)

I suppose you could try a workaround but I would like to keep the original logic somewhat present to avoid that. Original bug #451

@yeban

This comment has been minimized.

Show comment
Hide comment
@yeban

yeban Nov 6, 2014

Contributor

If stylesheet is from a different domain sheet.href will be null?

Contributor

yeban commented Nov 6, 2014

If stylesheet is from a different domain sheet.href will be null?

@yeban

This comment has been minimized.

Show comment
Hide comment
@yeban

yeban Nov 6, 2014

Contributor

Also, #451 mentions the source of security exception on Firefox to be L96. Is the conditional really needed in both _getStyleSheets & colorForBase?

Contributor

yeban commented Nov 6, 2014

Also, #451 mentions the source of security exception on Firefox to be L96. Is the conditional really needed in both _getStyleSheets & colorForBase?

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Nov 7, 2014

Contributor

I suppose it is only needed for _getStyleSheets. And I guess my technique of targetting resource:// urls was sort of overly specific too. I can change it to just use a try/catch the security exception.

Contributor

cmdcolin commented Nov 7, 2014

I suppose it is only needed for _getStyleSheets. And I guess my technique of targetting resource:// urls was sort of overly specific too. I can change it to just use a try/catch the security exception.

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Nov 7, 2014

Contributor

Let us know if there are any further issues. Interested in seeing the results of any new styles!

Contributor

cmdcolin commented Nov 7, 2014

Let us know if there are any further issues. Interested in seeing the results of any new styles!

@cmdcolin cmdcolin closed this Nov 7, 2014

@yeban

This comment has been minimized.

Show comment
Hide comment
@yeban

yeban Nov 8, 2014

Contributor

I think try ... catch approach is good and works for me. Thanks.

If you are only interested in resource:// urls and not in href being null, the following conditional probably would have worked for both of us as well: if (!(sheet.href && sheet.href.match("^resource:\/\/")) { ...}. Because the issue for me was the rejection of sheets with href==null in colorForBase.

Contributor

yeban commented Nov 8, 2014

I think try ... catch approach is good and works for me. Thanks.

If you are only interested in resource:// urls and not in href being null, the following conditional probably would have worked for both of us as well: if (!(sheet.href && sheet.href.match("^resource:\/\/")) { ...}. Because the issue for me was the rejection of sheets with href==null in colorForBase.

@cmdcolin cmdcolin added this to the 1.11.6 milestone Jan 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment