VisualEditor: Inspector of FocusNodes with embedded menu should not be embedded in some cases
Closed, ResolvedPublic

Description

Author: oliver.buchtala

Description:
Currently, if the menu for a FocusNode gets embedded, this also happens for the Inspector. It would be better to let the Inspector itself decide.

Details:

This gets executed when the menu for a focus node is shown. 'this.embedded' is set according to the size of the focus node:

https://github.com/wikimedia/VisualEditor/blob/356f325ab21731e0aa5c83f90fb4fa037cc60cff/modules/ve/ui/ve.ui.DesktopContext.js#L428-L435

OTOH, when the inspector gets opened, 'this.embedded' is not recomputed:

https://github.com/wikimedia/VisualEditor/blob/356f325ab21731e0aa5c83f90fb4fa037cc60cff/modules/ve/ui/ve.ui.DesktopContext.js#L420-L425

This has the consequence that the inspector is rendered over the focussed node.
IMO, at least the same embedded check should be done in the 'inspector' case.
Also the tail should be enabled accordingly.

To leave the inspector implementation even more control I would suggest to add something like this to the Inspector interface:

/**

  • @returns {boolean} */

Inspector.prototype.isEmbedded = function() {};


Version: unspecified
Severity: normal

bzimport set Reference to bz66542.
bzimport created this task.Via LegacyJun 12 2014, 6:15 PM
bzimport added a comment.Via ConduitJun 12 2014, 6:33 PM

oliver.buchtala wrote:

... or better:

/**

  • @returns {boolean} */

Inspector.prototype.isEmbeddable = function() {

return true;

};

A custom inspector can then override this to avoid embedding and existing implementations would not be influenced.

bzimport added a comment.Via ConduitJun 12 2014, 6:53 PM

oliver.buchtala wrote:

(In reply to Oliver Buchtala from comment #1)

Maybe it could look like this:

https://github.com/oliver----/VisualEditor/commit/26baf0bf57fb3fb23ce7ae5971b25f29ac8182db

bzimport added a comment.Via ConduitJun 12 2014, 6:58 PM

oliver.buchtala wrote:

(In reply to Oliver Buchtala from comment #2)

https://github.com/oliver----/VisualEditor/commit/
26baf0bf57fb3fb23ce7ae5971b25f29ac8182db

Sorry, this link was wrong.

https://github.com/oliver----/VisualEditor/commit/8c0e5acb6df5bc104165e9fbbf3f9b9959f89135

Jdforrester-WMF added a comment.Via ConduitJul 1 2014, 8:27 PM

Sorry for the slow triage here; between this and bug 67306 we think it'd be best to actively prevent the context menu from showing over an item that is inspectable (rather than just have it be a configuration), so I'm going to mark this one as the primary and the other as

Jdforrester-WMF added a comment.Via ConduitJul 1 2014, 8:28 PM

[Bah]

(In reply to James Forrester from comment #4)

Sorry for the slow triage here; between this and bug 67306 we think it'd be
best to actively prevent the context menu from showing over an item that is
inspectable (rather than just have it be a configuration), so I'm going to
mark this one as the primary and the other as

… as possibly a WONTFIX, rather than leave it be.

gerritbot added a comment.Via ConduitJul 1 2014, 8:28 PM

Change 143393 had a related patch set uploaded by Jforrester:
Never embed the context when an inspector is present

https://gerrit.wikimedia.org/r/143393

gerritbot added a comment.Via ConduitJul 1 2014, 8:50 PM

Change 143393 merged by jenkins-bot:
Never embed the context when an inspector is present

https://gerrit.wikimedia.org/r/143393

Esanders added a comment.Via ConduitAug 22 2014, 12:35 PM
  • Bug 67306 has been marked as a duplicate of this bug. ***
Esanders added a comment.Via ConduitAug 22 2014, 12:36 PM

Regressed.

gerritbot added a comment.Via ConduitAug 22 2014, 12:37 PM

Change 155704 had a related patch set uploaded by Esanders:
Never embed the context when an inspector is present

https://gerrit.wikimedia.org/r/155704

gerritbot added a comment.Via ConduitAug 22 2014, 5:17 PM

Change 155704 merged by jenkins-bot:
Never embed the context when an inspector is present

https://gerrit.wikimedia.org/r/155704

Add Comment