- 
                Notifications
    
You must be signed in to change notification settings  - Fork 35
 
Remove Dynamically Created Inline Styles from Map Layers #1463
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR eliminates dynamically created inline styles from map layers to comply with Content Security Policy restrictions. The change removes the JavaScript-based hover functionality that was creating inline styles and replaces it with CSS-only hover behavior.
- Removed 
changeVisibilityByElementId()function and related event handlers that created inline styles - Added CSS hover rules to handle menu visibility without JavaScript
 - Removed hardcoded "hidden" CSS classes from menu elements
 
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description | 
|---|---|
| MapLayerDroppable.tsx | Removed JavaScript functions that created inline styles and their corresponding event handlers | 
| MapLayerDroppable.scss | Added CSS hover rules to control menu visibility without inline styles | 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| 
           please test out with the test app in this repo and turn on the CSP settings that caught this  | 
    
          
 As discussed in this comment on the linked issue, there are some difficulties with testing with the specific CSP settings that caught this problem in the first place which are being worked on. As a way to get some testing done, I created   
And after my changes, we receive this output: This shows that map-layers should now be free of inline styles. We do not necessarily need to keep this script in the final pr, but it does at least show that there should now be no other instances of inline styles within map-layers.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
| 
           Quick update on testing with the csp - the csp generally forbids inline styles but has a list of "known" inline styles it allows. This generally explains the concerns mentioned here on why the issue does not seem to be wider reaching than it is. Working with @gedkek and @GintV to confirm some details about the error/presence of other errors. I'll update this PR with one more comment once those details are confirmed, but the changes made here should be all that is necessary to solve the problem.  | 
    
          
 I was able to do more testing with a template app using the CSP discussed, and I did not see any errors within map-layers after consuming the package with these changes in them. This PR should include all changes necessary to fully resolve the issue. As a note, the CSP being used here forbids all inline styles except for those strictly allowed by the list of known inline style hashes. This means that if any app using this or a similar CSP attempts to consume other packages from viewer-components-react, there is a chance that this issue will come back. Now that the testing has been sorted out and with the inclusion of the  It would be a large and tedious undertaking to replace all instances of inline styles within viewer-components-react at once. However, it might still be worth considering pre-emptively doing so given that they do violate this CSP and even MDN's recommended CSP.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Resolves #1389.
As previously addressed in this PR, some apps' content security policies do not allow inline styles. That PR attempted to fix the bug described in the attached issue by converting all instances of inline styles in map-layers with corresponding css classes, however it missed the inline styles created in the
changeVisibilityByElementId()function. This function was used to override existing styles in order to allow certain menu items to appear when hovering over an attached map layer. This PR addresses this issue by removingchangeVisibilityByElementId()and letting css handle the hover functionality. Attached below is a clip of the hover functionality working with these changes implemented.workingHover.mp4