Last modified: 2014-08-22 18:12:24 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T68542, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 66542 - VisualEditor: Inspector of FocusNodes with embedded menu should not be embedded in some cases
VisualEditor: Inspector of FocusNodes with embedded menu should not be embedd...
Status: RESOLVED FIXED
Product: VisualEditor
Classification: Unclassified
Editing Tools (Other open bugs)
unspecified
All All
: Normal normal
: VE-deploy-2014-07-03
Assigned To: Ed Sanders
:
: 67306 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-12 18:15 UTC by Oliver Buchtala
Modified: 2014-08-22 18:12 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Oliver Buchtala 2014-06-12 18:15:15 UTC
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() {};
Comment 1 Oliver Buchtala 2014-06-12 18:33:11 UTC
... 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.
Comment 2 Oliver Buchtala 2014-06-12 18:53:39 UTC
(In reply to Oliver Buchtala from comment #1)

Maybe it could look like this:

https://github.com/oliver----/VisualEditor/commit/26baf0bf57fb3fb23ce7ae5971b25f29ac8182db
Comment 4 James Forrester 2014-07-01 20:27:56 UTC
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
Comment 5 James Forrester 2014-07-01 20:28:25 UTC
[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.
Comment 6 Gerrit Notification Bot 2014-07-01 20:28:54 UTC
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
Comment 7 Gerrit Notification Bot 2014-07-01 20:50:17 UTC
Change 143393 merged by jenkins-bot:
Never embed the context when an inspector is present

https://gerrit.wikimedia.org/r/143393
Comment 8 Ed Sanders 2014-08-22 12:35:50 UTC
*** Bug 67306 has been marked as a duplicate of this bug. ***
Comment 9 Ed Sanders 2014-08-22 12:36:10 UTC
Regressed.
Comment 10 Gerrit Notification Bot 2014-08-22 12:37:37 UTC
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
Comment 11 Gerrit Notification Bot 2014-08-22 17:17:56 UTC
Change 155704 merged by jenkins-bot:
Never embed the context when an inspector is present

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

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links