Skip to content

fix: Make the tooltip visible in IE10+ #2452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 24, 2019
Merged

fix: Make the tooltip visible in IE10+ #2452

merged 1 commit into from
Aug 24, 2019

Conversation

SEWeiTung
Copy link
Contributor

@SEWeiTung SEWeiTung commented Aug 23, 2019

Notice:
This 'rect' is invisible as a container for the language symbol
to show the tooltip in both IE and Chrome, FF,ect.

For reasons of accessibility, user agents should always make the content
of the 'title' child element to the root svg element available to users.
However, this is typically done through other means than the tooltips
used for nested SVG and graphics elements, e.g., by displaying in a browser
tab.

See more at: https://svgwg.org/svg2-draft/struct.html#TitleElement

Issues and Subjects related to this are:
1、#2294.
2、#2403.
3、#2392.

@SEWeiTung
Copy link
Contributor Author

SEWeiTung commented Aug 23, 2019

@XhmikosR, Please have a review, thanks!

@fhemberger, Sorry to interrupt you, what about making @XhmikosR as a member of the Nodejs.org? It seems he has contributed a lot.... I'm not sure who has the right to do this? So this is just for confirm.

@fhemberger
Copy link
Contributor

@MaledongGit I think you can ask @MylesBorins or @Trott.

@SEWeiTung
Copy link
Contributor Author

@MylesBorins & @Trott, what about adding @XhmikosR as the member of Nodejs.org?You can see his great contributions in his list.
@fhemberger, thanks!

tab.
See more at: https://svgwg.org/svg2-draft/struct.html#TitleElement
-->
<rect id="lang-rect" x="0" y="0" width="512" height="512" fill="none" pointer-events="fill"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'd remove the comment. Just mention all this in the commit message.

Also, please follow the same attributes order that svgo outputs.

Finally, this doesn't work at all for me in any browser; the toggler does not open.

Copy link
Contributor Author

@SEWeiTung SEWeiTung Aug 23, 2019

Choose a reason for hiding this comment

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

Personally I'd remove the comment. Just mention all this in the commit message.

OK, fixed it.

Also, please follow the same attributes order that svgo outputs.

What does this mean?

Finally, this doesn't work at all for me in any browser; the toggler does not open.

What do you mean by "the toggler does not open"? Everything goes fine with me in my local master with the latest codes until now. Maybe you can have a try with npm run serve to see the final result.
And for the final result, you can see https://nodejs.org/en/ in both IE or Chrome, you can see that when you click the language symbol, the menu expands rightly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean with your patch the toggler does not open for me with any browser.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean?

For optimal gzip compression, the attributes are sorted. Here is your SVG with this opt:

<svg id="lang-picker-toggler" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 512 512">
  <rect id="lang-rect" width="512" height="512" x="0" y="0" fill="none" pointer-events="fill"/>
  <path d="M217.982 201.586h-64.499c-5.537 0-10.026 4.489-10.026 10.026s4.489 10.026 10.026 10.026h53.547c-4.72 25.263-26.935 44.446-53.547 44.446-30.037 0-54.473-24.436-54.473-54.473s24.436-54.473 54.473-54.473c14.55 0 28.229 5.667 38.518 15.955 3.916 3.916 10.264 3.916 14.178 0 3.916-3.916 3.916-10.264 0-14.178-14.077-14.077-32.791-21.829-52.697-21.829-41.094 0-74.525 33.431-74.525 74.525 0 41.094 33.431 74.525 74.525 74.525s74.525-33.431 74.525-74.525c.001-5.536-4.488-10.025-10.025-10.025z"/>
  <path d="M470.331 92.24H269.728l-26.935-81.355a10.025 10.025 0 00-9.518-6.875H41.669C18.693 4.01 0 22.703 0 45.679v332.412c0 22.976 18.693 41.669 41.669 41.669h203.145l27.073 81.369a10.026 10.026 0 009.513 6.861h188.932c22.976 0 41.669-18.693 41.669-41.669V133.909c-.001-22.976-18.694-41.669-41.67-41.669zM41.669 399.708c-11.919 0-21.616-9.697-21.616-21.616V45.679c0-11.919 9.697-21.616 21.616-21.616h184.364l70.691 213.516a.366.366 0 00.015.043l53.664 162.086H41.669zm295.78-116.433c.805 1.11 10.824 14.877 26.355 34.066-4.377 5.756-9.015 11.474-13.91 17.036l-29.712-89.74h87.441c-6.196 13.031-16.938 33.813-31.692 55.736-13.553-16.921-22.069-28.622-22.249-28.87-3.251-4.482-9.519-5.481-14.002-2.23-4.482 3.25-5.48 9.518-2.231 14.002zM265.946 419.76h75.162l-55.503 59.084-19.659-59.084zm226.002 46.561c0 11.919-9.697 21.616-21.616 21.616H304.575l67.015-71.339-.004-.003c.293-.312.571-.64.823-.991a10.025 10.025 0 001.39-9.022l-16.688-50.402c7.073-7.406 13.68-15.143 19.805-22.965 13.299 15.772 29.037 33.446 45.778 50.187 1.957 1.957 4.524 2.937 7.089 2.937s5.132-.979 7.089-2.937c3.916-3.916 3.916-10.264 0-14.178-17.461-17.461-34.013-36.244-47.687-52.632 21.251-30.503 35.033-59.504 40.535-71.954h21.454c5.537 0 10.026-4.489 10.026-10.026s-4.489-10.026-10.026-10.026h-66.173v-18.047c0-5.537-4.489-10.026-10.026-10.026s-10.026 4.489-10.026 10.026v18.046h-51.406l-37.178-112.292H470.33c11.919 0 21.616 9.697 21.616 21.616v332.412z"/>
</svg>

@XhmikosR
Copy link
Contributor

BTW IE11 shows the title for me just fine in master (the current live site), so this patch seems redundant.

@SEWeiTung
Copy link
Contributor Author

SEWeiTung commented Aug 23, 2019

BTW IE11 shows the title for me just fine in master (the current live site), so this patch seems redundant.

Why? I'm not sure there's something different in your local repository, but if you take a look at https://nodejs.org/en/, you will see that in IE11, if you move your mouse onto the language symbol, no tooltip shows unless my package is up there.

For running locally with a test, you can do it with npm run serve, but please make sure that all of your changes in your local hard disc don't effect this change ;)

Correct me if I take you wrong.

Here're the comparations, attention to the right-corner when the mouse is over the language switch symbol, the tooltip's difference.

【In https://nodejs.org/en/】
normalsite

【Locally test with npm run serve
test

@XhmikosR
Copy link
Contributor

Why? I'm not sure there's something different in your local repository, but if you take a look at https://nodejs.org/en/, you will see that in IE11, if you move your mouse onto the language symbol, no tooltip shows unless my package is up there.

OK, I see now where I got confused. I thought this was about the title when hovering on each language. What you describe indeed does not work in IE.

@XhmikosR
Copy link
Contributor

This is what I get here with your patch. Note that I'm clicking on the toggler but it doesn't open.

2019-08-23_16-27-55

@XhmikosR
Copy link
Contributor

For me the toggler does not open with Chrome, Firefox nor IE.

But I figured out why. It's because I was using DEFAULT_LOCALE=en. So weird, no JS errors or anything, the toggler just didn't work.

So the patch LGTM, just use the SVG I posted above and should be ready to go :)

@SEWeiTung
Copy link
Contributor Author

SEWeiTung commented Aug 23, 2019

This seems very weird……The default language is in server.js. I'll take a look if I'm free.

IE will not show any tooltip for the <title> element of the root <svg>
element.
So in order to show this, we have to make a trick by drawing a
transparent rectangle in which we put a tooltip.
Issues and Subjects related to this are:
1、#2294.
2、#2403.
3、#2392.
@XhmikosR
Copy link
Contributor

This just needs a reindendation and should be good to go.

@SEWeiTung SEWeiTung merged commit 689783f into nodejs:master Aug 24, 2019
@SEWeiTung
Copy link
Contributor Author

@XhmikosR, Well, I took a close look at the codes and it seems we can dynamically add multi languages through the env.DEFAULT_LOCALE (So this param should be in the whole environment,user-defined). And if your DEFAULT_LOCALE is only 'en', there will be ONLY 'en' built up, and there's only one language choosen for you now, and according to my logic, when your default language is 'en', you don't need to choose the same language becuase it's useless. So that's the reason why you cannot toggler.
It's not a bug I think. You can have a try by removing DEFAULT_LOCALE in your env (Make it as a undefined).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants