filter out hidden elements when copying code#13256
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces regex-based text filtering in the CopyButton component with a DOM-based approach that removes elements marked as hidden. Review feedback identifies a potential compilation error where the remove method is called on a generic Node type and suggests optimizing the node iterator to target elements specifically. It also notes that the newly added js_interop import would become redundant if these suggestions are adopted.
| final iterator = web.document.createNodeIterator(codeElement); | ||
| web.Node? currentNode; | ||
| while ((currentNode = iterator.nextNode()) != null) { | ||
| if (currentNode.isA<web.Element>() && | ||
| (currentNode as web.Element).getAttribute('aria-hidden') == | ||
| 'true') { | ||
| currentNode.remove(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation contains a bug and can be optimized:
- Bug:
currentNode.remove()is called on a variable of typeweb.Node?. Inpackage:web(whichuniversal_webwraps), theremove()method is defined on theChildNodeinterface (implemented byElementandCharacterData), but not on the baseNodeclass. This will likely cause a compilation error. - Optimization: By default,
createNodeIteratorvisits all nodes, including text nodes. Since you only need to check attributes on elements, usingweb.NodeFilter.SHOW_ELEMENTis more efficient. - Type Safety: Using a direct cast (safe when using
SHOW_ELEMENT) or the Dartisoperator for type promotion is cleaner than repeatedisAchecks.
| final iterator = web.document.createNodeIterator(codeElement); | |
| web.Node? currentNode; | |
| while ((currentNode = iterator.nextNode()) != null) { | |
| if (currentNode.isA<web.Element>() && | |
| (currentNode as web.Element).getAttribute('aria-hidden') == | |
| 'true') { | |
| currentNode.remove(); | |
| } | |
| } | |
| // Filter out hidden elements like the terminal sign or folding icons. | |
| final iterator = web.document.createNodeIterator( | |
| codeElement, | |
| web.NodeFilter.SHOW_ELEMENT, | |
| ); | |
| web.Node? currentNode; | |
| while ((currentNode = iterator.nextNode()) != null) { | |
| final element = currentNode as web.Element; | |
| if (element.getAttribute('aria-hidden') == 'true') { | |
| element.remove(); | |
| } | |
| } |
|
|
||
| import 'package:jaspr/jaspr.dart'; | ||
|
|
||
| import 'package:universal_web/js_interop.dart'; |
|
Visit the preview URL for this PR (updated for commit 2511c9c): https://flutter-docs-prod--pr13256-fix-copy-no-hidden-wcg56xbe.web.app |
sfshaza2
left a comment
There was a problem hiding this comment.
LGTM, but I will leave it to you to resolve the bot's complaints.
Fixes #13253
Presubmit checklist
of 80 characters or fewer.