Page MenuHomePhabricator

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

Details

Reference
bz66542

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:23 AM
bzimport set Reference to bz66542.

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.

oliver.buchtala wrote:

(In reply to Oliver Buchtala from comment #1)

Maybe it could look like this:

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

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

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

[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.

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

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

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

  • Bug 67306 has been marked as a duplicate of this bug. ***

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

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

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