Bugzilla – Full Text Bug Listing
| Summary: | Eager activation of Java editor breadcrumbs can cause unpredictable editor scrolling on mouse click | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Deepak Azad <deepakazad> | ||||||||||
| Component: | UI | Assignee: | Project Inbox <e4.ui-inbox> | ||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||
| Severity: | major | ||||||||||||
| Priority: | P3 | CC: | daniel_megert, emoffatt, hsoliwal, pwebster, remy.suen, tom.schindl | ||||||||||
| Version: | 1.0 | ||||||||||||
| Target Milestone: | 4.1 M3 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 179436 [details]
screenshot
This happens in more scenarios as well...
- I went to a reference of org.eclipse.ui.texteditor.AbstractTextEditor.PREFERENCE_SHOW_WHITESPACE_CHARACTERS
- Open (F3) to navigate to the declaration
- The editor opens as shown in the screenshot
- Now when I click anywhere the editor jumps to the top
Marking as major as it is getting quite irritating... (In reply to comment #0) > - Use 'Open Type (Ctrl + Shift + T)' to open a not yet opened type. => a java > editor opens up and something is selected in the breadcrumb as well - see > screenshot- it does not happen on 3.x Not totally sure why the breadcrumbs gets focus but here's the stack trace I get after I hit F3. Thread [main] (Suspended (breakpoint at line 212 in EditorBreadcrumb$1)) EditorBreadcrumb$1.handleEvent(Event) line: 212 EventTable.sendEvent(Event) line: 84 Display.filterEvent(Event) line: 1253 ToolBar(Widget).sendEvent(Event) line: 1052 ToolBar(Widget).sendEvent(int, Event, boolean) line: 1077 ToolBar(Widget).sendEvent(int) line: 1058 ToolBar(Control).sendFocusEvent(int) line: 2621 ToolBar(Widget).wmSetFocus(int, int, int) line: 2402 ToolBar(Control).WM_SETFOCUS(int, int) line: 4795 ToolBar.WM_SETFOCUS(int, int) line: 1362 ToolBar(Control).windowProc(int, int, int, int) line: 4232 Display.windowProc(int, int, int, int) line: 4886 OS.SetFocus(int) line: not available [native method] ToolBar(Control).forceFocus() line: 995 ToolBar(Control).setFixedFocus() line: 2983 ToolBar(Composite).setFixedFocus() line: 1039 Composite.setFixedFocus() line: 1037 Composite.setFixedFocus() line: 1037 Composite.setFixedFocus() line: 1037 Composite.setFixedFocus() line: 1037 Composite.setFixedFocus() line: 1037 Composite.setFixedFocus() line: 1037 Composite.setFixedFocus() line: 1037 Composite.setFixedFocus() line: 1037 CTabFolder(Composite).setFixedFocus() line: 1037 Composite(Control).fixFocus(Control) line: 953 Composite(Control).setVisible(boolean) line: 3410 CTabFolder.setSelection(int) line: 2818 CTabFolder.setSelection(CTabItem) line: 2775 StackRenderer.showTab(MUIElement) line: 510 LazyStackRenderer$1.handleEvent(Event) line: 74 UIEventHandler.handleEvent(Event) line: 41 EventHandlerWrapper.handleEvent(Event, Permission) line: 188 EventHandlerTracker.dispatchEvent(Object, Object, int, Object) line: 198 EventManager.dispatchEvent(Set<Entry<K,V>>, EventDispatcher<K,V,E>, int, E) line: 230 ListenerQueue<K,V,E>.dispatchEventSynchronous(int, E) line: 148 EventAdminImpl.dispatchEvent(Event, boolean) line: 139 EventAdminImpl.sendEvent(Event) line: 78 EventComponent.sendEvent(Event) line: 39 EventBroker.send(String, Object) line: 73 UIEventPublisher.notifyChanged(Notification) line: 58 PartStackImpl(BasicNotifierImpl).eNotify(Notification) line: 380 PartStackImpl(ElementContainerImpl<T>).setSelectedElement(T) line: 159 ModelServiceImpl.bringToTop(MWindow, MUIElement) line: 223 PartServiceImpl.activate(MPart, boolean) line: 412 PartServiceImpl.activate(MPart) line: 388 WorkbenchPage.activate(IWorkbenchPart) line: 417 WorkbenchPage.busyOpenEditor(IEditorInput, String, boolean, int, IMemento) line: 2199 WorkbenchPage.access$14(WorkbenchPage, IEditorInput, String, boolean, int, IMemento) line: 2173 WorkbenchPage$6.run() line: 2155 BusyIndicator.showWhile(Display, Runnable) line: 70 WorkbenchPage.openEditor(IEditorInput, String, boolean, int, IMemento) line: 2151 WorkbenchPage.openEditor(IEditorInput, String, boolean, int) line: 2135 WorkbenchPage.openEditor(IEditorInput, String, boolean) line: 2126 EditorUtility.openInEditor(IEditorInput, String, boolean) line: 367 EditorUtility.openInEditor(Object, boolean) line: 174 OpenAction.run(Object[]) line: 219 OpenAction.run(ITextSelection) line: 158 OpenAction(SelectionDispatchAction).dispatchRun(ISelection) line: 278 OpenAction(SelectionDispatchAction).run() line: 250 OpenAction(Action).runWithEvent(Event) line: 498 ActionHandler.execute(ExecutionEvent) line: 121 E4HandlerProxy.execute(IEclipseContext) line: 56 GeneratedMethodAccessor23.invoke(Object, Object[]) line: not available DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43 Method.invoke(Object, Object...) line: 618 MethodRequestor.execute() line: 52 InjectorImpl.invokeUsingClass(Object, Class<?>, Class<Annotation>, Object, PrimaryObjectSupplier, PrimaryObjectSupplier, boolean) line: 208 InjectorImpl.invoke(Object, Class<Annotation>, Object, PrimaryObjectSupplier) line: 185 ContextInjectionFactory.invoke(Object, Class<Annotation>, IEclipseContext, Object) line: 101 HandlerServiceImpl.executeHandler(ParameterizedCommand) line: 135 KeyBindingDispatcher.executeCommand(ParameterizedCommand, Event) line: 266 KeyBindingDispatcher.press(List, Event) line: 465 KeyBindingDispatcher.processKeyEvent(List, Event) line: 515 KeyBindingDispatcher.filterKeySequenceBindings(Event) line: 348 KeyBindingDispatcher.access$0(KeyBindingDispatcher, Event) line: 294 KeyBindingDispatcher$KeyDownFilter.handleEvent(Event) line: 76 EventTable.sendEvent(Event) line: 84 Display.filterEvent(Event) line: 1253 StyledText(Widget).sendEvent(Event) line: 1052 StyledText(Widget).sendEvent(int, Event, boolean) line: 1077 StyledText(Widget).sendEvent(int, Event) line: 1062 StyledText(Widget).sendKeyEvent(int, int, int, int, Event) line: 1103 StyledText(Widget).sendKeyEvent(int, int, int, int) line: 1099 StyledText(Widget).wmKeyDown(int, int, int) line: 1808 StyledText(Control).WM_KEYDOWN(int, int) line: 4499 StyledText(Control).windowProc(int, int, int, int) line: 4194 StyledText(Canvas).windowProc(int, int, int, int) line: 341 Display.windowProc(int, int, int, int) line: 4886 OS.DispatchMessageW(MSG) line: not available [native method] OS.DispatchMessage(MSG) line: 2460 Display.readAndDispatch() line: 3655 PartRenderingEngine$4.run() line: 684 Realm.runWithDefault(Realm, Runnable) line: 332 PartRenderingEngine.run(MApplicationElement, IEclipseContext) line: 593 E4Workbench.createAndRunUI(MApplicationElement) line: 104 Workbench$3.run() line: 546 Realm.runWithDefault(Realm, Runnable) line: 332 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 528 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149 IDEApplication.start(IApplicationContext) I feel like there's either an issue with SWT here or maybe JDT needs to change their code. The only reason this wasn't a problem in 3.x seems to be because the tool bar didn't get focus when you swapped between editors because the view's composite wasn't actually living under the tab folder. (In reply to comment #4) > The only reason this wasn't a problem in 3.x seems to be because > the tool bar didn't get focus when you swapped between editors because the > view's composite wasn't actually living under the tab folder. In fact, the tab items don't even have a control so there is nothing they can update the focus for. Since the tool bar takes focus when an editor is activated, you can _not_ begin typing as you switch between editors and are forced to activate the text area manually through alternate means. Would it be possible for JDT/Text to alter the focus handling code in the breadcrumb? It seems to me like anyone that tried to write their own presentation with a stack of editors would hit this problem due to SWT recursively granting focus to the first control of a composite. (In reply to comment #4) > view's composite wasn't actually living under the tab folder. Sorry, I do not have the background here - why is this different in 4.x ? (In reply to comment #6) > Since the tool bar takes focus when an editor is activated, you can _not_ begin > typing as you switch between editors and are forced to activate the text area > manually through alternate means. The toolbar can also take focus as a result of 'Navigate > Show in Breadcrumb' action, but there is no unpredictable editor scrolling after that. (In reply to comment #7) > Would it be possible for JDT/Text to alter the focus handling code in the > breadcrumb? It seems to me like anyone that tried to write their own > presentation with a stack of editors would hit this problem due to SWT > recursively granting focus to the first control of a composite. Don't know if this is useful - but the Toolbar is actually the second control in its parent composite. (In reply to comment #8) > > view's composite wasn't actually living under the tab folder. > Sorry, I do not have the background here - why is this different in 4.x ? I'm not sure about the "why", I suppose the argument is that it is more intuitive that way. When you look at a view, you would think its parent (or grandparent, or ancestor, or whatever) control is the tab folder, right? In 3.x, this is not the case, and is what people in-the-know call "the big lie". This was done in 3.x (and before) because not all windowing systems supported the reparenting of controls properly. So instead we'd arbitrarily set the bounds of a part's composite to be the size of the tab folder's client area making it seem as if it was housed under the tab folder...when in reality, it actually wasn't! > (In reply to comment #6) > The toolbar can also take focus as a result of 'Navigate > Show in Breadcrumb' > action, but there is no unpredictable editor scrolling after that. It's not yet clear to me what the difference is. It may be because you haven't actually swapped editors. > (In reply to comment #7) > Don't know if this is useful - but the Toolbar is actually the second control > in its parent composite. My mistake. It was naive of me to make the "first" claim. In any case, the code in SWT essentially recurses down to the tool bar and asks it take focus via forceFocus() and it does (return true successfully). If you check the Display filter that's in EditorBreadcrumb in 3.x, you will _not_ get focusGained() invoked when you swap between editors. This does happen in 4.x though (as illustrated by the stack trace in comment 3), from a simple call to CTabFolder's setSelection(CTabItem) method. (In reply to comment #8) > The toolbar can also take focus as a result of 'Navigate > Show in Breadcrumb' > action, but there is no unpredictable editor scrolling after that. This seems to be because the breadcrumb stores the previous selection when it has been granted focus and attempts to restore it when focus has been lost. In the single editor case (the scenario described above), the previous selection is whatever I want the breadcrumb to navigate to, and when I leave the breadcrumb and reactivate the text control...well, the selection hasn't actually changed. The F3 case is different though you end up opening a new editor. In this case, the breadcrumbs gets focus, it stores the "old" selection of (0,0), loses focus when you click in the editor, and tries to restore this (0,0) selection, causing the editor to scroll. When I have my caret on the element I am about to F3 to and then proceed to swap editors and hit F3, I am unable to reproduce the scrolling phenomena. The snippet below basically illustrates what I understand to be happening.
Run the code and then click inside the text control to grant it focus. Now alternate between the two tabs by clicking on them. You will see now that it is the toolbar that gets the focus event...which, isn't particularly out of the ordinary when you think about it given that it comes before the text control.
------------
Display display = new Display();
Shell shell = new Shell(display);
shell.setLayout(new FillLayout());
CTabFolder folder = new CTabFolder(shell, SWT.BORDER);
for (int i = 0; i < 2; i++) {
CTabItem item = new CTabItem(folder, SWT.CLOSE);
item.setText("Item " + i);
Composite composite = new Composite(folder, SWT.NONE);
composite.setLayout(new FillLayout());
ToolBar bar = new ToolBar(composite, SWT.FLAT);
bar.addFocusListener(new FocusAdapter() {
public void focusGained(FocusEvent e) {
System.out.println("toolbar gained focus"); //$NON-NLS-1$
}
});
ToolItem toolItem = new ToolItem(bar, SWT.PUSH);
toolItem.setText("a");
Text text = new Text(composite, SWT.MULTI);
text.addFocusListener(new FocusAdapter() {
public void focusGained(FocusEvent e) {
System.out.println("text control gained focus"); //$NON-NLS-1$
}
});
item.setControl(composite);
}
shell.pack();
shell.open();
while (!shell.isDisposed()) {
if (!display.readAndDispatch()) {
display.sleep();
}
}
display.dispose();
It works on 3.x, so I assume this is rather an issue in e4 rather than a in JDT which does not use any internal code from Platform UI. (In reply to comment #10) > The F3 case is different though you end up opening a new editor. In this case, > the breadcrumbs gets focus, it stores the "old" selection of (0,0), loses focus > when you click in the editor, and tries to restore this (0,0) selection, > causing the editor to scroll. When I have my caret on the element I am about to > F3 to and then proceed to swap editors and hit F3, I am unable to reproduce the > scrolling phenomena. Actually this (0,0) selection is an invalid selection, see org.eclipse.jdt.internal.ui.javaeditor.JavaEditor.JdtSelectionProvider.getSelection() and org.eclipse.jdt.internal.ui.javaeditor.JavaEditor.JdtSelectionProvider.markInvalid() (In reply to comment #7) > Would it be possible for JDT/Text to alter the focus handling code in the > breadcrumb? It seems to me like anyone that tried to write their own > presentation with a stack of editors would hit this problem due to SWT > recursively granting focus to the first control of a composite. I don't know what is the best solution for this problem - prevent the breadcrumb from getting focus by changing SWT behavior, e.g. not allowing CTabFolder to recursively grant focus to its children controls OR - handling the invalid selection more gracefully in JDT > - handling the invalid selection more gracefully in JDT and if we do this Bug 325948 will still remain :-) (In reply to comment #12) > It works on 3.x, so I assume this is rather an issue in e4 rather than a in JDT > which does not use any internal code from Platform UI. Eric and Paul, any ideas here? I currently don't see a way out of this unless we reimplement "the big lie". (In reply to comment #15) > > Eric and Paul, any ideas here? I currently don't see a way out of this unless > we reimplement "the big lie". It seems to me that the client can't tell the difference between a real SWT.Focus event (the user clicked there) and the one generated by the CTabFolders fix focus code. Maybe we need an enhancement in CTF so we can not-fix-focus and set that option in our stack renderer. That would get rid of the spurious focus event, which in the context of our part stacks is not useful (and can apparently cause trouble :-) PW A deferred event causes the breadcrumb to regain focus despite the fact that focus has already been granted to the text control. If you asynchronously grant focus to the text control in setFocus() then there doesn't appear to be any problems. org.eclipse.jdt.internal.ui.javaeditor.breadcrumb.EditorBreadcrumb$1.handleEvent(EditorBreadcrumb.java:216) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Display.filterEvent(Display.java:1253) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1052) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1077) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1058) at org.eclipse.swt.widgets.Control.sendFocusEvent(Control.java:2621) at org.eclipse.swt.widgets.Widget.wmSetFocus(Widget.java:2402) at org.eclipse.swt.widgets.Control.WM_SETFOCUS(Control.java:4795) at org.eclipse.swt.widgets.Control.windowProc(Control.java:4232) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4873) at org.eclipse.swt.internal.win32.OS.SetFocus(Native Method) at org.eclipse.swt.widgets.Control.forceFocus(Control.java:995) at org.eclipse.swt.widgets.Control.setFixedFocus(Control.java:2983) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1039) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Composite.setFixedFocus(Composite.java:1037) at org.eclipse.swt.widgets.Control.fixFocus(Control.java:953) at org.eclipse.swt.widgets.Control.setVisible(Control.java:3410) at org.eclipse.e4.ui.widgets.CTabFolder.setSelection(CTabFolder.java:2818) at org.eclipse.e4.ui.widgets.CTabFolder.setSelection(CTabFolder.java:2826) at org.eclipse.e4.ui.widgets.CTabFolder.onMouse(CTabFolder.java:1471) at org.eclipse.e4.ui.widgets.CTabFolder$2.handleEvent(CTabFolder.java:266) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1053) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4066) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3657) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:693) Ok, a few days of deep diving into SWT and here's the scoop: The basic issue is the result of the editor actually being a true child of the CTabFolder now (rather than a child of the shell in 3.x). The standard CTF logic will call 'hide' on the tab that is going to the background when a new tab becomes selected. The control being hidden had the focus so SWT has to pick a new focus and the logic it uses is to walk the children until one accepts focus. When the breadcrumb is up it is the first child that accepts focus so it *always* gets a FocusIn event, causing it to set its 'fIsActive' flag. This is not the end of the story though...when it gets focus a side-effect is that the new tab gets an 'Activate' event which we trap and use to activate the part in the WorkbenchPage. We call the part's 'setFocus' as part of this and even if we remove the code there so that is will always put the focus on the editor it still doesn't end up there. With a lot of help from Silenio we tracked this back to a 'tail test' in the SWT setFocus (setFixFocus) method; Remember that our call was triggered by an Activate generated during the breadcrumb's handling of its 'setFocus' so at the end of that call the focus is *not* in the breadcrumn. This leads SWT to determine that that setFocus didn't work so it continues, leading to it stomping our call to the part's setFocus... There are two approaches being evaluated at this time: - Change the SWT behavior to allow re-setting the focus to a different control while processing 'setFocus' for a different control. This change will also need a small fix to the EditorBreadCrumb so that the 'FocusIn' listener only reacts if the editor is the active editor (this prevents it setting the fIsActive when it gets the spurious event when SWT is resetting the focus). - Change SWT to allow the focus behavior to be over-ridden. This would allow us to override the Composite we use to wrap a part in the ContributedPartRenderer and have it re-direct the focus using our API. While this is likely the optimal fix for us it's new API and (justifiably) a much harder sell to SWT. Created attachment 180239 [details]
Patch to inhibit the FocusIn listener unless the editor is active
This prevents unexpected effects generated by the CTF's setting the focus to the breadcrumb while bringing up the new editor.
NOTE: there is a minor regression here. If you have a split editor area and click on the breadcrumb of a non-active editor then the 'fIsActive' will remain false (meaning that clicking on another editor's tab and back will end up with the focus in the editor. If the user actually uses the editor then all is well.
Created attachment 181757 [details] Patch to intercept 'setFocus' and delegate to the Part's impl Now that a Composite can override 'setFocus' (see Bug 328290) this patch traps the attempt to set the focus to the Composite containing an MPart and uses DI to invoke the '@Focus' method on the part's implementation. This is a nice clean fix, thanks to the SWT folks for their quick response...(we'd have been pretty well sunk without this). Committed in >20101026. Applied the patch. I've kicked this around and it seems to work for me but it might be worth checking out on other platforms... Marking FIXED... (In reply to comment #20) > Now that a Composite can override 'setFocus' (see Bug 328290) this patch traps > the attempt to set the focus to the Composite containing an MPart and uses DI > to invoke the '@Focus' method on the part's implementation. The EPartService also will invoke the annotated method. Does this mean the method will be invoked twice? I suspect that you are correct. Do you have any ideas on how we might be able to get around this? Since both calls will (certainly should) result in the same widget being granted focus I suspect that the impact is minimal. Verified in I20101029-0251. *** Bug 325948 has been marked as a duplicate of this bug. *** Looking at the fix I think the overloading of setFocus() could lead to an endless loop so you should add a guard in the method so that I can't be entered while you call the @Focus method. |
Created attachment 179304 [details] screenshot Steps - Have at least one java file open - Have Breadcrumb enabled - Use 'Open Type (Ctrl + Shift + T)' to open a not yet opened type. => a java editor opens up and something is selected in the breadcrumb as well - see screenshot- it does not happen on 3.x - Now click towards the bottom of the editor => the editor suddenly scrolls up!