Skip to content
This repository has been archived by the owner on Oct 26, 2021. It is now read-only.

Fix marble hover/dragging styles for Firefox #19

Merged
merged 1 commit into from
Jul 28, 2015
Merged

Fix marble hover/dragging styles for Firefox #19

merged 1 commit into from
Jul 28, 2015

Conversation

kaplas
Copy link
Contributor

@kaplas kaplas commented Jun 7, 2015

This PR

I tried to debug why the previous drop shadow did not work in Firefox. It seems that Firefox handles measurements against SVG viewBox differently than Webkit-based browsers. I did not encounter any exactly same kind of bug report in the Internet, but I did found this one, where the symptomps were very similar, but the DOM structure did not match. There a commenter actually mentions that Chrome and WebKit would be actually be handling viewBox-based filtering wrongly. Don't know who is right and who is wrong, but at least by taking into account the number of related FF bugs, it is safe to assume that the situation is not going to resolve anytime soon.

The only alternative that I found was to implement a SVG-native drop-shadow filter element, which is then just linked by its ID to the related elements.

  • Hack-fixes user-select for Firefox

For some reason -moz-user-select style attribute does not end up to the DOM via virtual-dom rendering. That is very weird, taking into account that a similar vendor prefixed version works in Chrome. However it is possible to define that property via a normal CSS file or with the FF CSS dev tool panel. If you have idea what is going on here, please share your wisdom.

As a hack-fix I propose that for this single thing an additional CSS clause could be added to main.less.

I understand

...that in both of these cases abstraction is leaking from components to the outside world. However, these were the only fixes that I could come up with which reliably fix these FF problems. If you have some ideas on how these fixes should be stored in a more "componentish" way (instead of having those in index.html and main.less), I'm more than happy to change this PR.

And BTW, after when we have got this PR fixed, I have more global-leakage-related bugs and questions waiting for you; I'm very sorry if I end up tearing apart your component-based dream of the future ;-)

<feMergeNode in="SourceGraphic"/>
</feMerge>
</filter>
</svg>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-2 space indentation for the </svg> please

@staltz
Copy link
Owner

staltz commented Jun 7, 2015

Super thanks for the PR and fighting down this bug.

For some reason -moz-user-select style attribute does not end up to the DOM via virtual-dom rendering. That is very weird

We need to figure out why. If we can't then we should open an issue on virtual-dom. It "should work".

And BTW, after when we have got this PR fixed, I have more global-leakage-related bugs and questions waiting for you; I'm very sorry if I end up tearing apart your component-based dream of the future ;-)

Curious about it.

stroke: Colors.black,
fill: color
},
isDraggable && isHighlighted ? marbleElevation1Style : {}),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-2 space indentation for lines 34--37.

@kaplas
Copy link
Contributor Author

kaplas commented Jun 7, 2015

@staltz I made a codepen about that -moz-user-select + virtual-dom problem:
http://codepen.io/kaplas/pen/rVmyXP?editors=001
The bug seems to be reproducible. As this is literally the first time I'm using virtual-dom, could you check that I haven't done any Stupid User Errors there. If it looks legit, I will submit an issue for virtual-dom.

@staltz
Copy link
Owner

staltz commented Jun 7, 2015

Seems like there are no Stupid User Errors.

@kaplas
Copy link
Contributor Author

kaplas commented Jun 7, 2015

OK, I submitted that one: virtual-dom/issues/#265.

Would you like me to remove that hack-fix, or just add an appropriate todo comment near to the fix, or what?

@staltz
Copy link
Owner

staltz commented Jun 7, 2015

A comment would be enough. 👍

@staltz staltz merged commit 23c5806 into staltz:master Jul 28, 2015
@staltz
Copy link
Owner

staltz commented Jul 28, 2015

@kaplas I finally had time to work on rxmarbles and merge this. Thanks 👍

@kaplas
Copy link
Contributor Author

kaplas commented Jul 28, 2015

Sorry that I did not have the time to do the changes you requested... :(

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

Successfully merging this pull request may close these issues.

2 participants