Bugzilla – Full Text Bug Listing
| Summary: | [formatting] HTML cleanup document should offer compress empty element tags | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Source Editing | Reporter: | Amy Wu <for.work.things> | ||||||||||||||||||
| Component: | wst.html | Assignee: | Ian Tewksbury <itewksbu> | ||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Nitin Dahyabhai <thatnitind> | ||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||
| Priority: | P3 | CC: | nsand.dev, thatnitind | ||||||||||||||||||
| Version: | 1.5 | Keywords: | helpwanted | ||||||||||||||||||
| Target Milestone: | 3.2 M5 | Flags: | nsand.dev:
review+
|
||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Amy Wu
Considering for 3.0, although there may not be time. reassigning to inbox Care should be taken when considering that in html, but not xhtml, some tags can simply be: <br> <li> Then maybe this should wait for Bug 258676 so that there is an XHTML content type that I can use to differentiate between XHTML and HTML. Created attachment 154593 [details]
Enhancment Patch
It turns out HTML already had the preference for this there was just no UI for it. So this patch copies the UI implementation for the tag compression from the CleanupDialogXML and puts it into the CleanupDialogHTML and then duplicates the actual logic for compressing tags from the org.eclipse.wst.xml.core.internal.cleanup.ElementNodeCleanupHandler#compressEmptyElementTag and puts it org.eclipse.wst.html.core.internal.cleanup.ElementNodeCleanupHandler as well. That's all it took to get the magic working.
P.S. working on some JUnits Created attachment 154654 [details]
Enhancment and JUnits Patch
Same as the last patch only this time has some added junits.
Created attachment 154767 [details]
Enhancment and JUnits Patch Update 1
Sometimes helps if after writing awesome JUnits you hook them into the JUnit suite....
Hey, Ian. While I think overall the patch is good, I've noticed that <div> tags in particular do not like to be compressed. I think this may have to do with isContainer() returning true for ElementImpl. Can this be an additional check before compressing the tags? (In reply to comment #9) > Hey, Ian. While I think overall the patch is good, I've noticed that <div> tags > in particular do not like to be compressed. I think this may have to do with > isContainer() returning true for ElementImpl. Can this be an additional check > before compressing the tags? Do you mean DIV tags should not be compressed, or they should be and for you they are not being compressed? I ask because when I run it on a document with empty div tags they all get compressed. But if you were meaning to say some tags should be compressed and those are the ones whose "isContainer" returns true then that functionality could easily be added. (In reply to comment #10) > (In reply to comment #9) > > Hey, Ian. While I think overall the patch is good, I've noticed that <div> tags > > in particular do not like to be compressed. I think this may have to do with > > isContainer() returning true for ElementImpl. Can this be an additional check > > before compressing the tags? > > Do you mean DIV tags should not be compressed, or they should be and for you > they are not being compressed? > > I ask because when I run it on a document with empty div tags they all get > compressed. But if you were meaning to say some tags should be compressed and > those are the ones whose "isContainer" returns true then that functionality > could easily be added. Nevermind, i now noticed that <div/> is reported with a "no end tag" error. I'll track down the exact circumstances that say a tag can not be compressed and add that as a condition. Created attachment 154974 [details]
Enhancment and JUnits Patch Update 2 (Version 1)
There are handy helper utility methods in org.eclipse.wst.html.core.internal.validate.CMUtil for checking to see if a CMElementDeclaration for a IDOMElement says its allowed to omit its end tag.
This version of the patch copies the two needed helper methods from CMUtil and puts them in ElementNodeCleanupHandler becuase currently CMUtil is package protected. I will attach another patch that makes CMUtil public so ElementNodeCleanupHandler can access it.
I can see a valid argument for both solutions so I will let you pick your favorite.
Created attachment 154975 [details] Enhancment and JUnits Patch Update 2 (Version 2) Same functionality as the "version 1" patch except this one makes CMUtil public so ElementNodeCleanupHandler can access its helper methods. Less duplicated code this way but it also means making CMUtil public. So as I said in comment 12 I can arguments for both solutions. Created attachment 154976 [details] Enhancment and JUnits Patch Update 3 (Version 1) Same as patch described in comment #12 except I moved the model query call inside the if for checking if the tag is even one to be considered for compressing. This saves on some execution time. Created attachment 154977 [details] Enhancment and JUnits Patch Update 3 (Version 2) Same as patch described in comment #13 except I moved the model query call inside the if for checking if the tag is even one to be considered for compressing. This saves on some execution time. Hey Ian, I think CMUtil.isEndTagOmissible() is being used incorrectly to determine if a tag can be changed from <tag></tag> to <tag />. My understanding of isEndTagOmissible() is that it allows for things like <br>, <meta>, <li> etc. All of these can be written as such, with no closing or self-closing tag. I think what you may need to do is if the document is not XHTML, then check if the Element is a container. If it's not a container, then I think you can compress it. Created attachment 155369 [details]
Enhancement and JUnits Patch Update 4
Changed logic to if the tag is either an XHTML tag or not a container then it will be compressed.
Changes checked in. Thanks, Ian. |