gwtproject / gwt

GWT Open Source Project
http://www.gwtproject.org
1.53k stars 377 forks source link

CellTree in Chrome loses focus by keybord navigation #8866

Open dankurka opened 9 years ago

dankurka commented 9 years ago

Originally reported on Google Code with ID 8911

Found in GWT Release: 2.6.1

Encountered on OS / Browser: Windows 7, Mac OS, Chrome(37), Safari

Detailed description (please be as specific as possible):

The CellTree widget can be selected by keyboard, with the arrow keys.
This works in FF but is broken in Chrome and Safari, if, and only if i go from Level
0 to Level 1 with the DOWN-Arrow key in my cell tree, then i can not go any further
with the keyboard.

Example: Starting with selection on the top line in the following diagram and pressing:
down, down, down -> stuck and the first L1 entry
Example 2: Starting with selection at the bottom line and pressing: up, up, up, up,
up -> works as expected

+Tree L0
+Tree L0
-Tree L0
 |Tree L1
 |Tree L1
+Tree L0

Digging into the problem shows that Chrome issues a blur event, which takes focus completly
away from the cell tree after the selection model sets the selection to the new entry.
Firefox has also the blur event, but it issues also an extra focus event.

Shortest code snippet which demonstrates issue (please indicate where
actual result differs from expected result):
    /**
     * Das Modell dieses Baumes erlaubt LeadDTO und PersonDTO anzuzeigen.
     * 
     * <pre>
     *   > Lead1 Name
     *   > Lead2 Name
     *   v Lead3 Name
     *     Person1 Name
     *     Person2 Name
     * </pre>
     */
    private class LeadTreeModel implements TreeViewModel {

        private final SingleSelectionModel selectionModel;

        public LeadTreeModel(SingleSelectionModel selectionModelLead) {
            selectionModel = selectionModelLead;
        }

        @Override
        public <T> NodeInfo<?> getNodeInfo(T value) {
            if (value == null) {
                // LEVEL 0.
                // We passed null as the root value. Return the leads.

                // Create a cell to display a lead.
                Cell<LeadDTO> leadCell = new AbstractCell<LeadDTO>() {
                    @Override
                    public void render(Context context, LeadDTO value, SafeHtmlBuilder sb) {
                        sb.appendEscaped(value.getName());
                    }

                };
                // der tree hat ein eigenes icon, das in TreeResources überschrieben wird.

                // Return a node info that pairs the data provider and the cell.
                return new DefaultNodeInfo<LeadDTO>(leadDataProvider, leadCell, selectionModel,
null);
            } else if (value instanceof LeadDTO) {
                // LEVEL 1.
                ListDataProvider<PersonDTO> dataProvider = new ListDataProvider<PersonDTO>(((LeadDTO)
value).getPersons());

                AbstractCell<PersonDTO> cell = new AbstractCell<PersonDTO>() {

                    @Override
                    public void render(Context context, PersonDTO value, SafeHtmlBuilder sb) {
                        if (value.getFirstname() == null) {
                            sb.appendEscaped(value.getLastname() + ", ");
                        } else {
                            sb.appendEscaped(value.getFirstname() + " " + value.getLastname() + ", ");
                        }
                        int i = 0;
                        for (AssetclassDTO assetClass : value.getAssetclasses()) {
                            if (i > 0) {
                                sb.appendEscaped(", ");
                            }
                            sb.appendEscaped(assetClass.getNameShortEN()); // FIXME Sprachabhängigkeit
                            i++;
                        }
                        if (value.getJoblevel().getNameEN().length() > 0) {
                            sb.appendEscaped(" (" + value.getJoblevel().getNameEN() + ")"); // FIXME Sprachabhängigkeit
                        }

                    }
                };

                // Return a node info that pairs the data provider and the cell.
                return new DefaultNodeInfo<PersonDTO>(dataProvider, cell, selectionModel, null);
            }

            return null;
        }

        @Override
        public boolean isLeaf(Object value) {
            if (value instanceof PersonDTO)
                return true;
            return false;
        }

    }

Using the Tree:
[...]

        selectionModel.addSelectionChangeHandler(new SelectionChangeEvent.Handler() {
            @Override
            public void onSelectionChange(SelectionChangeEvent event) {
                if (selectionModel.getSelectedObject() instanceof LeadDTO) {
                    presenter.onLeadSelectionChanged((LeadDTO) selectionModel.getSelectedObject());         
                } else if (selectionModel.getSelectedObject() instanceof PersonDTO) {
                    presenter.onContactPersonSelectionChanged((PersonDTO) selectionModel.getSelectedObject());              
                }

            }
        });

        TreeViewModel model = new LeadTreeModel(selectionModel);
        tree =
                new CellTree(model, null, INJECTOR.getResources().treeResources(), GWT.<CellTreeMessages>
create(CellTreeMessages.class),
                        DEFAULT_RESULTSPERPAGE);
        tree.setKeyboardSelectionPolicy(KeyboardSelectionPolicy.BOUND_TO_SELECTION);

        initWidget(UI_BINDER.createAndBindUi(this));

        scrollPanel.setWidget(tree);
[...]

Workaround if you have one:
none found

Reported by Singu.B on 2014-09-23 15:05:57

dankurka commented 9 years ago
I'm not seeing this in Showcase [1] so perhaps there's something different about the
example.

[1] http://samples.gwtproject.org/samples/Showcase/Showcase.html#!CwCellTree

(I also tested against trunk.)

Reported by skybrian@google.com on 2014-09-24 22:40:46

dankurka commented 9 years ago
I can not reproduce it in the online showcase too, but i have found a reliable way to
break this example [1].

In the NodeInfo inner Class change these lines for level 0 and level 1: 
return new DefaultNodeInfo<Composer>(dataProvider, cell);
to
return new DefaultNodeInfo<Composer>(dataProvider, cell, selectionModel, null);

and change the declaration of the selectionModel more generic to make the compiler
happy
private final SingleSelectionModel<String> selectionModel = new SingleSelectionModel<String>();
to
private final SingleSelectionModel<Object> selectionModel = new SingleSelectionModel<Object>();

it doesnt matter if the KeyboardSeletionPolicy is ENABLED or BOUND_TO_SELECTION, but
its more easy to see with the last option.

Steps to reproduces:
ENABLED:
  Click on "Beethoven", see you can freely move with the arrow keys
  Click on "Concertos", now the yellow marking does not move as the tree lost focus
  Same happens if you now click on "No. 1 - C"
  Again: the focus is only lost if one comes from above.

BOUND_TO_SELECTION:
  Click on "Beethoven" to give the Tree focus
  Move down
  Stuck on Concertos as focus is lost.
  Same from level 1 to 2

[1] http://www.tutorialspoint.com/gwt/gwt_celltree_widget.htm

Reported by Singu.B on 2014-09-25 11:15:29

dankurka commented 9 years ago
Are there any news on this bug? 

Reported by Singu.B on 2014-10-17 09:13:14

dankurka commented 9 years ago
There have been a couple of fixes to keyboard navigation in CellTree but I don't know
if it fixes the same thing.

Reported by skybrian@google.com on 2014-10-17 15:49:41

dankurka commented 9 years ago
As a few months have passed and I am still interested in seeing this bug fixed I request
hereby a status update.

Reported by Singu.B on 2015-01-19 14:34:48

dankurka commented 9 years ago
As far as I know, nobody is working on CellTree at the moment. We need a volunteer.

Reported by skybrian@google.com on 2015-01-20 23:10:30

ibaca commented 9 years ago

I submit a patch to fix this.

https://gwt-review.googlesource.com/#/c/13830/

The problem is that saveChildState() { ... savedView.ensureAnimationFrame().removeFromParent() ... } (line 408) fires blur events on chrome. This blurs events are usually ignored using isRefreshing=true but looks like replaceChildren has a bug and the saveChildState was outside the isRefreshing=true...false block (line 246). The similar method replaceAllChildren has its saveChildState call inside the isRefreshing=true...false block (line 211).