Opened 10 years ago

Closed 10 years ago

#11490 closed Bug (fixed)

Menubutton does not work in source mode

Reported by: Wiktor Walc Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.3.3
Component: General Version: 4.3
Keywords: Cc:

Description

After this commit:

https://github.com/ckeditor/ckeditor-dev/commit/ddf9074ad04d18d5ba6a8dcc1a9162936e455bc2

menubutton stopped working in source mode. After pressing the menubutton, the dropdown menu does not open when the editor is in source mode.

Firefox reports: TypeError: this.getSelection(...) is null

Chrome reports: Uncaught TypeError: Cannot call method 'getStartElement' of null

The attached plugin can be used to reproduce this issue.

Attachments (2)

test-master.zip (2.3 KB) - added by Wiktor Walc 10 years ago.
11490.html (2.1 KB) - added by Piotrek Koszuliński 10 years ago.

Download all attachments as: .zip

Change History (21)

Changed 10 years ago by Wiktor Walc

Attachment: test-master.zip added

comment:1 Changed 10 years ago by Wiktor Walc

Status: newconfirmed

comment:2 Changed 10 years ago by Marek Lewandowski

Reason for that issue is that menu plugin calls editor#elementPath() method twice, in onShow() and show() methods, each of these calls causes now mentioned exception when we're in sourceview mode. Fix may be simply implemented in elementPath method, checking current mode.

comment:3 Changed 10 years ago by Piotrek Koszuliński

cc

comment:4 Changed 10 years ago by Wiktor Walc

Milestone: CKEditor 4.3.3

comment:5 Changed 10 years ago by Piotrek Koszuliński

Editor#elementPath() should return null if getSelection() returned null, because getSelection() returns null in other modes than wysiwyg. Then null values have to be handled correctly in menubutton code.

comment:6 Changed 10 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:7 Changed 10 years ago by Marek Lewandowski

Pushed solution to t/11490 at dev and t/11490 at tests.

comment:8 Changed 10 years ago by Marek Lewandowski

Status: assignedreview

comment:9 Changed 10 years ago by Piotrek Koszuliński

Status: reviewreview_failed

Selection should not be retrieved if startNode is passed.

comment:10 Changed 10 years ago by Marek Lewandowski

Pushed to t/11490.

comment:11 Changed 10 years ago by Marek Lewandowski

Status: review_failedreview

comment:12 Changed 10 years ago by Piotrek Koszuliński

Status: reviewreview_failed

The tests is in a wrong place - it's a test for elementsPath method, not for sourcearea.

comment:13 Changed 10 years ago by Marek Lewandowski

Status: review_failedreview

But it's elementpath function of core editor object, it's not a part of elementspath plugin. So what's the other proposition? should we make core code aware of sourcearea plugin?

comment:14 Changed 10 years ago by Piotrek Koszuliński

Editor#elementPath is an editor's method so its tests belongs to editor's API tests. And it's not aware of sourcearea plugin - it's aware of editor.mode property. You can test it by overriding editor.mode value or by using sourcearea for that to have perfectly realistic test.

Additionally, it would be good to have two menubutton integration tests - one for each mode.

comment:15 Changed 10 years ago by Marek Lewandowski

Status: reviewassigned

comment:16 Changed 10 years ago by Marek Lewandowski

Status: assignedreview

Tests repushed, as of integration test with both sourcearea and wysiwygarea i've so far wasted 2 hrs, with buggy effect so i'm dropping it. It's a candidate for separate ticket.

Changed 10 years ago by Piotrek Koszuliński

Attachment: 11490.html added

comment:17 Changed 10 years ago by Piotrek Koszuliński

Status: reviewreview_passed

Force pushed dev and tests after rebasing and improving both branches.

comment:18 Changed 10 years ago by Piotrek Koszuliński

Summary: menubutton does not work in source modeMenubutton does not work in source mode

comment:19 Changed 10 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

fixed with git:c96cd55 at dev master, and e846582 at tests master

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy