Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#13141 closed defect (fixed)

Tree: selection box is limited only by the visible width of the tree

Reported by: Sergey Kravchenko Owned by: bill
Priority: high Milestone: 1.8
Component: Dijit - LnF Version: 1.6.1
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

Seen by scrolling the tree to the right while node(s) are selected.

Before:

After:

Attachments (5)

showCase.html (1.3 KB) - added by Sergey Kravchenko 8 years ago.
showcase
before-scroll.png (3.3 KB) - added by Sergey Kravchenko 8 years ago.
after-scroll.png (3.6 KB) - added by Sergey Kravchenko 8 years ago.
dijitInline.gif (10.7 KB) - added by bill 8 years ago.
screenshot from test_Tree.html after applying dijitInline class
TreeNode_dijitInline.patch (9.9 KB) - added by bill 7 years ago.
Patch against [28627] to attempt to fix problem on horizontally scrollable trees where blue background for hovered and selected TreeNodes doesn't extend all the way to the right, in the case where some TreeNode are wider than the Tree itself (producing a horizontal scrollbar on the tree). It only works though for the widest nodes in the Tree. For the shorter nodes, the blue background extends to the end of the text, but not all the way to the right. Also, I had to remove the left and right borders to make the min-width: 100% work correctly, since box-sizing: border-box doesn't work on inline-block elements, at least on firefox. But it seems like those 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. IE6 and IE7 don't support min-width, so the patch has no effect there, but it does work on IE8+.

Download all attachments as: .zip

Change History (16)

Changed 8 years ago by Sergey Kravchenko

Attachment: showCase.html added

showcase

Changed 8 years ago by Sergey Kravchenko

Attachment: before-scroll.png added

Changed 8 years ago by Sergey Kravchenko

Attachment: after-scroll.png added

comment:1 Changed 8 years ago by bill

Component: DijitDijit - LnF
Description: modified (diff)
Summary: The selection box is limited only by the visible width of the treeTree: selection box is limited only by the visible width of the tree (claro)

comment:2 Changed 8 years ago by Douglas Hays

dijit/templates/TreeNode.html has
div dojoAttachPoint="rowNode" class="dijitTreeRow"
which should probably be
div dojoAttachPoint="rowNode" class="dijitInline dijitTreeRow"

Changed 8 years ago by bill

Attachment: dijitInline.gif added

screenshot from test_Tree.html after applying dijitInline class

comment:3 Changed 8 years ago by bill

Unfortunately using dijitInline on that node causes problems when the text's width is less than the width of the tree itself:

screenshot from test_Tree.html after applying dijitInline class

comment:4 Changed 7 years ago by bill

Owner: set to bill
Status: newassigned

I think the fix is to set a width, rather than having width=auto. I've seen similar problems with Tooltip where it gets wrapped at where the viewport's right border would be if the viewport was scrolled all the way to the left.

comment:5 Changed 7 years ago by bill

Resolution: patchwelcome
Status: assignedclosed

I've thought about this and although I agree it's a problem, I just don't see a good solution.

The basic problem is that the Tree itself limited to (for example) 150px, whereas the TreeNodes can be from 50px to 500px, depending on the length of text of each node. To be precise, each TreeNode may be limited to 150px, but they can have overflowing text making them appear wider.

We can set the selection box to match the width of the Tree, like it is now, via width: 100%. Alternately we can easily make each nodes' selection box match the length of that node (like tundra does) using dijitInline. But it's hard make every nodes' selection box width == the scrollWidth of the Tree, since the scrollWidth changes based on the longest visible tree row, and that potentially changes on every expand/collapse of a parent TreeNode. So it seems to require measuring the width of every node every time a new node is shown or hidden (via expand/collapse).

I'm going to mark this as patchwelcome. If someone has a good patch or approach I'd be very interested to see it.

Changed 7 years ago by bill

Attachment: TreeNode_dijitInline.patch added

Patch against [28627] to attempt to fix problem on horizontally scrollable trees where blue background for hovered and selected TreeNodes doesn't extend all the way to the right, in the case where some TreeNode are wider than the Tree itself (producing a horizontal scrollbar on the tree). It only works though for the widest nodes in the Tree. For the shorter nodes, the blue background extends to the end of the text, but not all the way to the right. Also, I had to remove the left and right borders to make the min-width: 100% work correctly, since box-sizing: border-box doesn't work on inline-block elements, at least on firefox. But it seems like those 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. IE6 and IE7 don't support min-width, so the patch has no effect there, but it does work on IE8+.

comment:6 Changed 7 years ago by bill

Milestone: tbd1.8
Resolution: patchwelcome
Status: closedreopened
Summary: Tree: selection box is limited only by the visible width of the tree (claro)Tree: selection box is limited only by the visible width of the tree

(Changing summary since the same problem exists in the other themes for the hover effect.)

I've been rethinking this problem and I'm ready to (reluctantly) check in a javascript solution. A pure CSS solution would be better, but a javascript solution seems better than nothing. Hopefully I won't regret the decision.

comment:7 Changed 7 years ago by bill

Resolution: fixed
Status: reopenedclosed

In [28631]:

Fix problem on horizontally scrollable trees where blue (or orange) background for hovered and selected rows doesn't extend all the way to the right, in the case where the tree has a horizontal scrollbar because some rows are wider than the Tree itself.

I couldn't figure out a way to do this via CSS; it's complicated since some rows are narrower than the Tree (ie, narrower than the Tree's offsetWidth), whereas some are wider, and we want the background for both types of rows to stretch the correct distance. So I added javascript sizing, done after any synchronous or asynchronous drawing operation completes: ex: expanding a single tree node, set("paths", ...), or collapseAll(). If the performance turns out to be unacceptable may need to rethink the solution. (The code is not optimal, but seems to perform well enough.)

I moved the indentation to a sub-node of the dijitTreeRow, and on claro, I removed the left and right borders. Together, this makes the sizing easier: I can set this.rowNode.style.width directly rather than calling domGeometry.setMarginBox(this.rowNode, ...). 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.

Fixes #13141 !strict.

comment:8 Changed 7 years ago by bill

In [29065]:

Fix Tree problem on initial display when it gets a horizontal scrollbar.

The expand operation of the root node was causing a vertical scrollbar to appear, reducing the available width. Since the expand happens in postCreate(), before this._startup is set, startPaint() ignored it. Because the idea was that we would adjust the widths on startup(). However, by the time the expand finishes startup() has already completed, and we to adjust the widths manually.

Fixes #15577, refs #13141 !strict.

comment:9 Changed 7 years ago by bill

In [29390]:

Avoid race condition accessing this.rootNode before it's set. Refs #13141 !strict.

comment:10 Changed 7 years ago by bill

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:11 Changed 7 years ago by bill

In [29952]:

Make Tree extend _CssStateMixin to get the dijitTreeRtl class set on RTL trees. Needed on IE8 (see tests/Bidi.html) or else the Tree is on the wrong side in the RTL TabContainer. Refs #13141 !strict.

Note: See TracTickets for help on using tickets.