147189 - Ability hide empty #text nodes (at least in the Inspector Sidebar)

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement, P2)

Description

[Artem Goutsoul]

The sidebar is mostly used to inspect pages, and often those empty #text nodes
are useless information. When you are a web developer you are only interested in
the nodes that you can control with css.

The tree would become a lot more meaningful and easier to read if it was
possible to have a toggle button to hide all empty text nodes and comment nodes.

Comment 1

[timeless]

*** Bug 177111 has been marked as a duplicate of this bug. ***

Comment 2

[Alex Vincent [:WeirdAl]]

I recommend WONTFIX for this one.  I am a web developer, and I'm *very* 
interested in nodes I can control with DOM.  Unfortunately, these nodes may be 
insignificant to CSS, but they are very significant in the DOM (see bug 26179 
for a lengthy discussion).

RFE is deprecated in favor of severity: enhancement.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Um, what?  This doesn't want to remove them completely from inspector.  This
just wants a toggle for them.  Just like the toggle for anonymous nodes.  I
thought I picked this up already.  I've been working on this.  I think this will
be a very useful feature.  Don't use it if you don't want it.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed fix

Amazing how much easier it is to inspect documents with this...

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

+      <menuitem id="item:toggleWhitespaceNodes"

don't you have two menuitems with that ID in the same file, then?

\ No newline at end of file
maybe you should add one.


also... does this patch add this feature both in the sidebar and in the
"stand-along" window?

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

- You are forgetting one of the basic rules of XML: The 'id' attribute is NOT an
IDREF (for getElementById) in any language except those that explicitly define
it.  X?HTML does so.  Here we do not, so my usage of it is perfectly legal, in
fact necessary if I want the popup menu to work in both places (View menu and
the drop down right arrow thingy menu that the sidebar uses, so the answer to
your third question is yes.)

- I did add a new line implicitly.  The file structure before was:

  "last line of actual code<nl>
   a bunch of spaces<nl>
   a bunch of spaces"

By deleting those last two lines, you can see that there now is a new line at
the end.

Comment 7

[Alec Flett]

Comment on attachment 105743 [details] [diff] [review]
Proposed fix

seems fairly reasonable. sr=alecf

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 105743 [details] [diff] [review]
Proposed fix

Checked this part in.  I'm going to leave this bug open though because this is
a slightly over-zealous implementation.  It will even hide the text node in:
<pre>	</pre>

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

I should also note that I'll have that fixed soon.  I just wanted to get this
portion in.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Don't hide the whitespace in <pre> </pre>

So two things I noticed while doing this:

- nsStyleText::WhiteSpaceIsSignificant() does not return true for
-moz-pre-wrap.	I think that is probably a bug...

- XP_IS_SPACE in nsTextFragment.h does not return true for '\r' characters.

Should either of those behaviors be changed?

Comment 11

[Boris Zbarsky [:bzbarsky]]

+  if (!win) {

How about doing this check before doing all the work of getting the nodeValue
and such?

+  // No frame really means that we are a child of a node that should
+  // not contain content. 

That comment is bogus.  No frame means the node or some ancestor is display:none.

+  nsAString::const_iterator start, end;

nsAutoString::const_iterator, since you know the string type....

Would it make sense to check the style context for the node in the case when it
only contains whitespace, even if it has no frame?

> - XP_IS_SPACE in nsTextFragment.h does not return true for '\r' characters.

There should be no \r chars in the DOM.  We should probably be asserting if
there are any -- that means the parser fucked up.

Not sure about -moz-pre-wrap; check the current callers of that function to see
how they use it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106120 [details] [diff] [review]
Don't hide the whitespace in <pre>    </pre>

Oh, and be warned that this may have building issues and may require moving to
nsInspectorUtils if it horks OSX or some linux builds....

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Better fix

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 106150 [details] [diff] [review]
Better fix

If this does happen to break some builds, I may just use getComputedStyle.... 
Of course it does in fact build for me on gcc 3.2 which is quite picky for me,
though we all know how PMSy the macs can get...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106150 [details] [diff] [review]
Better fix

> +  presContext->ResolveStyleContextFor(textContent, nsnull,

Why null for the parent context?

I have to wonder... why not just use the functions inspector already has that
properly get a style context?

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: "More better" fix

Because it didn't correctly get the style context in my case, and I didn't
change it.  Done now.  I twiddled a few more things around, like now my meat
and potatos function takes nsIDOMCharacterData which is probably what it should
be taking anyway...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106184 [details] [diff] [review]
"More better" fix

+  presShell->GetPrimaryFrameFor(textContent, &frame);

why bother?  Just use GetStyleContextForContent to start with and get the
struct off the style context.  That's what ::GetStyleData(nsIFrame*, ..) ends
up doing...

+      (const nsStyleText*)styleContext->GetStyleData(eStyleStruct_Text);

::GetStyleData(styleContext, &text);

(see nsIStyleContext.h)

looks good othewise!

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Do less work.

Man, you really know a patch sucks when you get 2 review- from the same person
all on one patch...  :)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106187 [details] [diff] [review]
Do less work.

r=bzbarsky conditional on actually fixing the 

+      (const nsStyleText*)styleContext->GetStyleData(eStyleStruct_Text);

thing.... ;)

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in.

Comment 21

[rbs]

Attachment: testcase: <table> + some MathML

There was a little leftover/oversight which is making the feature not effective
when empty text nodes have no associated frames. This is what happens with
<table>:

  <table> <!-- note that there are empty inter-tag whitespace nodes... -->
    <tr>
       <td>...</td>
    </tr>
  </table>

The inter-tag whitespace nodes are showing up even the menu option is selected.
This also happens with MathML in general because inter-tag whitespace nodes are
insignificant throughout MathML. As a result, looking at MathML in the DOM
inspector shows all these useless empty text nodes. I will attach a little
patch to fix the leftover.

Comment 22

[rbs]

Attachment: patch to catch ignorable text nodes that don't have frames

Comment 23

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 116273 [details] [diff] [review]
patch to catch ignorable text nodes that don't have frames

>+  *aReturn = PR_TRUE;

Not that it matters a whole lot or anything, but I'd appreciate you moving this
to after the early return for no window.  Putting it in an |else{}| block after
the |if (frame)| is a pretty good place.  Thanks for catching this.  r=caillon

Comment 24

[rbs]

Attachment: patch - use |else {}|

OK, switched to |else {}|. I hesitated doing that earlier and instead opted for
a minimalist patch.

Comment 25

[rbs]

Comment on attachment 116276 [details] [diff] [review]
patch - use |else {}|

Checked in.