Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16132 closed defect (fixed)

dijit.Tree superfluous DOM

Reported by: Wouter Hager Owned by: bill
Priority: undecided Milestone: 1.9
Component: Dijit Version: 1.8.0
Keywords: 1.8.2 Cc: mikeb
Blocked By: Blocking:

Description

Since 1.8 dijit.Tree has indentNodes. I consider this superfluous markup that makes the tree less transparent and harder to style. Please consider revising.

Attachments (8)

treeScrollTest.html (2.3 KB) - added by Wouter Hager 7 years ago.
Dijit.Tree 1.6.1 scrolling test
treeScrollTest.2.html (2.3 KB) - added by bill 7 years ago.
updated test to have short label too (to make sure highlighting goes all the way to the right for that label too)
treeScrollFix.2.patch (8.1 KB) - added by Wouter Hager 7 years ago.
improved patch from trunk
treeScrollFix.3.patch (9.7 KB) - added by Wouter Hager 7 years ago.
i forgot the templates…
treeScrollFix.patch (6.3 KB) - added by Wouter Hager 7 years ago.
clean patch
treeScrollTest2.html (2.0 KB) - added by Wouter Hager 7 years ago.
test for patch
treeScrollFix4.patch (6.6 KB) - added by bill 7 years ago.
treeScrollFix5.patch (8.1 KB) - added by bill 7 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 7 years ago by Wouter Hager

I see this has to do with #13141. I don't see the problem with setting node labels to blocks, which I could previously do and really need for my designs based on tree. I'd have to see the whole problem, but I hardly think a javascript solution is the way to go here. Or it least it should be optional.

comment:2 Changed 7 years ago by Wouter Hager

I quote #13141: "It seems like those left/right borders would be unwanted in general anyway, specifically when the Tree is inside a box such as the left-hand pane of a mail web-app, and thus there's already a left and right border flanking the tree."

I can't imagine I'm the only one who has left/right borders in their design.

comment:3 Changed 7 years ago by bill

Owner: changed from bill to Wouter Hager
Status: newpending

If you have a better solution to #13141 I'd be happy to hear it. If you want to see the problem with setting node labels to blocks, just look at the screenshots attached to #13141.

comment:4 Changed 7 years ago by Wouter Hager

Status: pendingnew

I think you could set .dijitTreeContainer { float:left } and .dijitTreeRow { display:block }

I.E. not treerows should not be inlined.

see http://stackoverflow.com/questions/4964041/css-white-spacenowrap-horizontal-scroll-bug

comment:5 Changed 7 years ago by Wouter Hager

Well not entirely correct of course. I'll test some more.

comment:6 Changed 7 years ago by Wouter Hager

Yup there it is. Add .dijitTreeRow { clear:both } The only thing left to do is resize the tree to the container on load. If this is an acceptable solution you can remove _adjustWidths, the indent nodes, and set the padding on the rowNode again.

Tested on Firefox, Chrome and IE9.

comment:7 Changed 7 years ago by bill

Hmm, well it would be great if just adding .dijitTreeRow { clear:both } solved the problem. Did you try it on the test case attached to #13141?

comment:8 Changed 7 years ago by Wouter Hager

I tried it with 1.8.1r3, using a similar case as #13141. I removed _startPaint, _adjustWidths, and the indent node from the node template. I added this CSS:

    .dijitTreeContainer {
    	float:left;
    }
    .dijitTreeRow {
    	display:block;
    	clear:both;
    }

There is one problem, for nodes that are shorter than their parent node the width still has to be set. I'll look into _adjustWidths.

comment:9 Changed 7 years ago by Wouter Hager

Nevermind then this is useless. How about not scrolling horizontally? It's ugly enough as it is.

comment:10 Changed 7 years ago by Wouter Hager

If you have all tree nodes inherit width you only need to set the domNode's width in _adjustWidths. In case you'd like to keep it this way... But since it's programmatic anyway I don't see why you couldn't read padding and border too.

comment:11 Changed 7 years ago by Wouter Hager

So I was on the right track after all, but it's even simpler. If you want the float:left to work you need to set it to a wrapper div for the tree. Set the wrapper width to auto when it scrolls and to inherit when it doesn't.

Changed 7 years ago by Wouter Hager

Attachment: treeScrollTest.html added

Dijit.Tree 1.6.1 scrolling test

comment:12 Changed 7 years ago by Wouter Hager

So here is the test. The wrapper could be added to the tree template, and then resize detect could be used to test if it scrolls.

Changed 7 years ago by bill

Attachment: treeScrollTest.2.html added

updated test to have short label too (to make sure highlighting goes all the way to the right for that label too)

comment:13 Changed 7 years ago by bill

Very interesting. So you are saying to add extra nodes to the Tree template, in particular the

<div id="wrapper" style="float:left;width:auto">

and then I suppose another wrapper node around that to enforce the specified width? It needs to work so that Tree.resize({w: 300}) will adjust Tree.domNode.

comment:14 Changed 7 years ago by Wouter Hager

No, the float:left can also be set on the tree itself. Please consider the patch.

Changed 7 years ago by Wouter Hager

Attachment: treeScrollFix.2.patch added

improved patch from trunk

Changed 7 years ago by Wouter Hager

Attachment: treeScrollFix.3.patch added

i forgot the templates...

Changed 7 years ago by Wouter Hager

Attachment: treeScrollFix.patch added

clean patch

Changed 7 years ago by Wouter Hager

Attachment: treeScrollTest2.html added

test for patch

comment:15 Changed 7 years ago by bill

Thanks, I'd be happy to consider the patch but you need to submit a CLA, can you do that?

comment:16 Changed 7 years ago by Wouter Hager

I submitted a CLA before.

comment:17 Changed 7 years ago by bill

Ok thanks! This looks a lot nicer than havingr to query the width of each node, but I'll try it tomorrow.

A few things:

  • The float:left needs to be in dijit.css rather than the test file.
  • I also wonder if the .dijitTree class can and should remain on the the outermost node, since that matches most other widgets.

comment:18 Changed 7 years ago by Wouter Hager

Considering the baseclass I agree.

I just wanted to add that I still think it's not likely that lots of people will have horizontal scrolling, cause it's just not good design.

Also, it took me a lot of time to find out in the first place what happened to the DOM (I just started reworking my codebase from 1.6.1 to 1.8). As I extend from a lot of widgets it's essential to know right away what's changed, without having to go through the whole code. Do you think it's possible to arrange for a "major changes" document in the future? I already mailed this to the dojo-interest list, but I didn't get the response I was hoping for. Might have been I wasn't clear enough, or that I should file it under complaints.dojotoolkit.org...

comment:19 in reply to:  18 Changed 7 years ago by bill

Milestone: tbd1.9
Owner: changed from Wouter Hager to bill
Status: newassigned

Replying to wshager:

Considering the baseclass I agree.

I just wanted to add that I still think it's not likely that lots of people will have horizontal scrolling, cause it's just not good design.

I guess so, although surely it's impossible to avoid sometimes given that many Trees contain user defined data of arbitrary width, and that users may adjust their browser font size/zoom.

Also, it took me a lot of time to find out in the first place what happened to the DOM (I just started reworking my codebase from 1.6.1 to 1.8). As I extend from a lot of widgets it's essential to know right away what's changed, without having to go through the whole code. Do you think it's possible to arrange for a "major changes" document in the future?

It already exists: the last section in the release notes for each release, labeled "Migration Notes". The 1.8 release notes have a section for Tree too, including template changes: http://livedocs.dojotoolkit.org/releasenotes/1.8#tree. Sorry, I should have added a note there for the indentNode change.

comment:20 Changed 7 years ago by bill

wshager - Thanks for the patch, hopefully we are almost there. I tried to adjust your patch given the issues I listed above:

  • The float:left needs to be in dijit.css rather than the test file.
  • I also wonder if the .dijitTree class can and should remain on the the outermost node, since that matches most other widgets.

I then tested on themeTester.html on chrome, but it's not working. If you make the left pane narrow enough, you can highlight "United States Of America" and scroll to the right, but the highlighting doesn't extend all the way.

Also, "South America" ends in a second column on the right, rather than underneath "North America".

Can you work on it a bit more?

Changed 7 years ago by bill

Attachment: treeScrollFix4.patch added

comment:21 Changed 7 years ago by Wouter Hager

Only the first container should float. Could you try

.dijitTree > .dijitTreeContainer {
float:left;
}

That should fix it. I can't test it in themeTester right now but my test in chrome looks fine.

Also, I'm not sure every situation will go as expected when setting role and classes on the containerNode, but perhaps you could enlighten me here.

Last edited 7 years ago by Wouter Hager (previous) (diff)

comment:22 Changed 7 years ago by bill

Oh, the TreeNode's also have a node with the class dijitTreeContainer (which should really be called dijitTreeNodeContainer). Thanks, didn't realize that.

OK, that explains it, I will change the name. (Note that ".dijitTree > .dijitTreeContainer" won't work correctly on older versions of IE.)

Last edited 7 years ago by bill (previous) (diff)

Changed 7 years ago by bill

Attachment: treeScrollFix5.patch added

comment:23 Changed 7 years ago by bill

OK wshager... well, it's working in many cases, but not for the simple case. Try test_Tree.html and you'll see that the highlighting only extends the width of the largest visible TreeNode. It should extend the width of the entire viewport.

comment:24 Changed 7 years ago by Wouter Hager

The viewport has no width specified, so from my point of view the behavior is not wrong. An option would be to specify width:100% instead of width:inherit.

Wouter

comment:25 Changed 7 years ago by Wouter Hager

But then there are other problems of course. I'll think about it.

comment:26 Changed 7 years ago by Wouter Hager

Setting width:100% instead of inherit seems to work.

in _adjustWidths:

this.containerNode.style.width = "auto"; this.containerNode.style.width = this.domNode.scrollWidth > this.domNode.offsetWidth ? "auto" : "100%";

comment:27 Changed 7 years ago by bill

Cc: mikeb added

Good, that seems to render correctly.

Your patch is breaking keyboard support for the Tree, as can be seen by running Tree_a11y.html or other tests. Presumably because you added data-dojo-attach-event="onkeypress:_onKeyPress" into the Tree template. I'll assume that's a merge error on your part.

Also, I kept the role=tree etc. on Tree.domNode, rather than moving them to Tree.containerNode. CC'ing mikeb in case that causes the problem listed in #12135.

comment:28 Changed 7 years ago by bill

Resolution: fixed
Status: assignedclosed

In [29824]:

Refactor of fix to #13141 to get highlighting to work correctly when a Tree is horizontally scrolled. Fixes #16132, refs #13141 !strict. Thanks to wshager (CLA on file) for the improved code!

comment:29 Changed 7 years ago by bill

Keywords: 1.8.2 added

PS: This change should possibly be backported to the 1.8 branch, since it's updating (and presumably improving) code that was added in 1.8.0. OTOH it's a big change, so it would be nice to put it through a real beta (ie the 1.9 beta) before releasing it upon the world.

Note: See TracTickets for help on using tickets.