Skip to content

Conversation

@Seebiscuit
Copy link

We test a global config variable mxClient.NO_FO in one key area of the CellRenderer:

if (mxClient.IS_SVG && mxClient.NO_FO && shape.dialect !== DIALECT_SVG) {

This variable is responsible for detecting if the current client supports the <foreign-object /> svg element. The <foreign-object /> element is the substrate used by HTML labels. If the NO_FO variable is set to false, the <foreign-object /> element is not rendered in the SVG output and non-string labels are not rendered.

Currently, the initialization test for mxClient.NO_FO does the following comparison:

     document.createElementNS(
        'http://www.w3.org/2000/svg',
        'foreignObject'
      ) !== '[object SVGForeignObjectElement]'

But the output of document.createElementNS(...) is an SVGElement which will always pass the condition above. This PR simply requests the stringified representation of this SVGElement which will match the RHS.

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.
The condition is taken from the original mxgraph code: https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/mxClient.js#L194
It seems that the condition about Opera is completely wrong, at least for the latest Opera versions.
Running Opera 81.0.4196.60 on Ubuntu 20.04.3. The navigator.userAgent value is
'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.69 Safari/537.36 OPR/81.0.4196.60' so the condition is always false too. And foreignObject are used for htmlLabels and Opera displays the label without any issue.
Opera has been using Chromium from years (2013-07-02) and the userAgent value has been updated when switching to Chromium. See https://help.opera.com/en/opera-version-history/
So I also suggest we remove the Opera condition as well.

josiahhaswell added a commit to aire-ux/mxgraph that referenced this pull request Nov 29, 2021
@josiahhaswell
Copy link

Fyi this is fixed in release https://www.npmjs.com/package/@aire-ux/mxgraph as of 4.2.6. aire-ux#2. Bit of a messy PR as it also integrates typed-mxgraph

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks for your contribution @Seebiscuit
About my comment about the Opera browser, I have created #67 and this will be managed later.

@tbouffard tbouffard merged commit 8d1c0de into maxGraph:development Dec 1, 2021
@aminosbh aminosbh mentioned this pull request Jun 4, 2022
14 tasks
@tbouffard tbouffard added the bug Something isn't working label Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants