Fix for keeping lists structure

java/org/chromium/distiller/webdocument/WebText.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.distiller.webdocument;
 
 import com.google.gwt.dom.client.Element;
 import org.chromium.distiller.DomUtil;
 import org.chromium.distiller.TreeCloneBuilder;
 import org.chromium.distiller.labels.DefaultLabels;
@@ skipping 51 matching lines @@
             Node parentClone = getTextNodes().get(0).getParentElement().cloneNode(false);
             parentClone.appendChild(clonedRoot);
             clonedRoot = parentClone;
         }
 
         // Make sure links are absolute and IDs are gone.
         DomUtil.makeAllLinksAbsolute(clonedRoot);
         DomUtil.stripIds(clonedRoot);
         DomUtil.stripFontColorAttributes(clonedRoot);
 
+        // Since LI Tag is being wrapped by a pair of {@link WebTag}s,
+        // we only need to get the innerHTML, otherwise
+        // LI tag would be duplicated.
+        Element elementClonedRoot = Element.as(clonedRoot);
         if (textOnly) {
-            return Element.as(clonedRoot).getInnerText();
+            return elementClonedRoot.getInnerText();
+        } else if (elementClonedRoot.getTagName().equals("LI")) {

[wychen, 2015/08/05 19:46:15]

UL and OL might need to use innerHTML as well.
Even though they shouldn't according to spec, we can't rely on web developers
following the spec, especially when it looks fine.

One possible malformed HTML snippet:

<ol>
  <li>one</li>
  outside
  <li>two</li>
</ol>

If all the text blocks are content, the distilled result would be:

<ol>
  <li>one</li>
  <ol>outside</ol>
  <li>two</li>
</ol>

Then the result is visually different.

[Marcelo Correa, 2015/08/05 20:31:24]

Yes, You are right. We might need to prevent that.

But I was wondering, How about we delegate this verification to another class?
Instead of WebText test if element is LI, OL, UL or another future elements, 
We ask to the WebTag itself.
For example:
WebTag.shouldUseOnlyInnerText(e.getTagName());
And WebTag has the allowed elements that should get the innerHTML.
It's just an idea to avoid writing more logic in the WebText instance.
What do you think?
What name for the method should we pick? Should we delegate this logic to WebTag
or another class? 

[wychen, 2015/08/06 00:04:52]

It would be cleaner to aggregate the tag list in one place. DomConverter can
also use it.
I think it could be a static method of WebTag.
As for the naming, maybe WebTag.canBeNested()? I'm not good at naming. :p

[Marcelo Correa, 2015/08/06 01:17:59]

That makes two of us :P

+            return elementClonedRoot.getInnerHTML();
         }
-        return Element.as(clonedRoot).getString();
+        return elementClonedRoot.getString();
     }
 
     public List<Node> getTextNodes() {
         return allTextNodes.subList(start, end);
     }
 
     public void addLabel(String s) {
         labels.add(s);
     }
 
@@ skipping 40 matching lines @@
     }
 
     public void setGroupNumber(int group) {
         groupNumber = group;
     }
 
     public int getGroupNumber() {
         return groupNumber;
     }
 }