Skip to content
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

Safari: look of the extension in some sites #135

Closed
monzug opened this issue Oct 29, 2018 · 36 comments
Closed

Safari: look of the extension in some sites #135

monzug opened this issue Oct 29, 2018 · 36 comments
Labels
bug safari safari-specific issues verified

Comments

@monzug
Copy link

monzug commented Oct 29, 2018

  1. in poesialatina.it, lookup button is smaller than usual size and the text is in black.
    actually all buttons have the text in black and not white

screen shot 2018-10-29 at 4 34 20 pm

  1. in http://thelatinlibrary.com/plautus/mercator.shtml, the Reskin options in Options panel are not alligned
@monzug monzug added the bug label Oct 29, 2018
@monzug monzug added the safari safari-specific issues label Oct 29, 2018
@balmas balmas assigned irina060981 and unassigned balmas Oct 29, 2018
@irina060981
Copy link
Member

the same as for
#136

Different priority for extension styles.

@irina060981
Copy link
Member

irina060981 commented Oct 31, 2018

Ok, I have re-read the basic documentation and Apple Team defines priority this way (source):

  1. Your Safari App Extension styles
  2. The webpage author's styles
  3. The webpage author's styles that are declared as !important
  4. Your Safari App Extension styles that are declared as !important

At each stage, a new definition overrides any previous definition. Style properties in your injected style sheets are added to existing page style properties, but your styles don’t override existing page styles unless you declare the new ones as !important.

So we are getting this terrible styles for Safari when someone defines there styles directly to tags.

I think - may be we wil create an additional safary-style file with importants for tags inside our ID attributes?
What do you think, @balmas and @kirlat ?

@balmas
Copy link
Member

balmas commented Oct 31, 2018

I think we can do that, but we should make sure that whatever we apply !important to is fully scoped to the alpheios namespace.

@kirlat
Copy link
Member

kirlat commented Oct 31, 2018

I hate important! with my whole heart because I was in position of needing to override them. So I prefer not to use them, if possible. For me it's like a nuclear option. 💣 ☠️

Maybe we could do away without importants by adding some id (#alpheios) to the root element of the host site with JS and then defining our styles as #alpheios .some-other-selectors? This combination should have a pretty high specificity to override whatever page creators defined.

But if that will not work (I'm not very familiar with all variety of sites out there), then probably important! is our last resort.

Or maybe I'm overcautious? Should we have the last word in how the page would look like? Who will need to be able to override our styles? Theme creators? But still, maybe it's better not to escalate to important if possible ❓

@balmas
Copy link
Member

balmas commented Oct 31, 2018

Yes I think maybe the first step is reviewing to make sure all of the elements we need to style in our UI are fully described by classes which can be traced back to the id of the parent alpheios ui component element?

@irina060981
Copy link
Member

Unfortunately, It seems to me that this problem couldn't be solved using any ID attributes or classes.
I will show you it in the following example:

@irina060981
Copy link
Member

Let's look at the button's style behaviour in Chrome and Safari (Lookup button on the panel):

** Chrome **:
image

Button looks ok, because styles applied this way:
Upper our injected styles as it has more weight because of classes:
image

And lower Bootstrap classes applied to a tag
image

@irina060981
Copy link
Member

And now Safari

screen shot 2018-11-01 at 09 49 25

It has another styling because if another line-height, color

Let's look as styles were applied:

Upper are bootstrap styles according to Safari App Extension injected styles order:
screen shot 2018-11-01 at 09 51 11

And only lower we could find our styles overwritten
screen shot 2018-11-01 at 09 51 54

If all stylesheets were applied as normal - our styles had the bigger weight then styles for a tag.
But It is not so.

And in Apple documentation - they offer to use !important - as it is the only way to give more weight to our styles.

P.S. Apple is unique in the thoughts about Web development :)

@kirlat
Copy link
Member

kirlat commented Nov 1, 2018

Wow! From reading Apple's docs where it was saying that

At each stage, a new definition overrides any previous definition.
I was under impression that this means a source order: an order in which rules are inserted into a final set of page's CSS rules. But specificity rules would remain as they are defined by W3C.

However, this seems not: it looks like Safari totally ignores CSS specificity and apply rules in its own Safari-specific order, combining both CSS specificity rules and "Safari-specific" ones. I think that's crazy! It's like Apple ignoring W3C specs and trying to come up with something of its own. That's clearly a "think different" way of things 🙂, and not in a good way, on my opinion !

So then, I guess, they forces us into an important land. I don't see how to do styling without it.

@kirlat
Copy link
Member

kirlat commented Nov 1, 2018

@irina060981, thanks for detailed clarification of how the rules are applied in Safari!

@irina060981
Copy link
Member

@kirlat , yes, it was really surprising for me too :) I think all safari app extention technology is an original approach to creating webextensions.

@kirlat
Copy link
Member

kirlat commented Nov 1, 2018

I think that even with Safari behaving the way it does we should not introduce any important styles into our webextension stylesheets. Then it means we need to have two sets of CSS files: one for webextension, and another one for Safari. The only difference between them is that Safari version should have !important after every rule, or, at best, after some rules that we absolutely need to redefine this way.

We definitely should use some script for that. I was thinking about PostCSS as it is a most widely accepted (as of now) way to post-process CSS files. There is even a PostCSS loader for Webpack: https://github.com/postcss/postcss-loader.

Maybe we can use something like postcss-important (https://www.npmjs.com/package/postcss-important). What do you think? Are there any other tools that will do the job?

@irina060981
Copy link
Member

My suggestion is the same - produce two style files - for webextension and for safari, also I think we should

  • refactor styles a little and wrap all our definitions into our ID attributes
  • I like your suggestion with PostCSS addition to webpack with important flag!

@balmas
Copy link
Member

balmas commented Nov 1, 2018

Agree with both points. We need to make sure all of our styles only apply to Alpheios ui elements and only use !important for Safari. I really like the idea of using postcss-important to add the !important flag when building for Safari.

@irina060981
Copy link
Member

I have faced with problems while applying postcss-important plugin:
it is very old (3 years ago) and completely oudated,
I have found a similiar plugin - postcss-safe-important - but it is outdated partly. (1 year ago)

I was not able to find any working plugins for Postcss to add important flag.

I will try to update it to use with our webpack/postcss version - I need to solve the following problems:

  1. we have scss and the current implementation can convert the css from vue components, but couldn't correctly parse scss from style folder (as it doesn't compile it to css)

  2. it has some limits (for example 100vh - 40px !important - fails all the style converting process)

So the task becomes a little bigger than I expected and will take more time unfortunately.

@balmas
Copy link
Member

balmas commented Nov 15, 2018

Ok. Maybe it's worth taking a step back to think about other approaches to this problem as alternatives to investing time in the post-css-important plugin? I'm coming up blank right now unfortunately.

@kirlat
Copy link
Member

kirlat commented Nov 15, 2018

I will try to get with some alternative solutions in the next couple of days, maybe there is something somewhere. It would be a maintenance nightmare to keep two set of similar files for different browsers.

@irina060981
Copy link
Member

There are really several problems here:

  1. we are not currently using postcss - we use different loaders

  2. I couldn't create a clear webpack task that produces only css - it will produce css and js (because webpack - is js bundler) - so build and build-safari will both create the same js, but different css

  3. But we need important near some styles for known problem sites and may be adding important will make as rearrange the order of styles. And may be we need more important flags for other problem sites we doesn't yet know.

So I think may be custom PostCSS plugin is not a bad idea
Will think about other solutions tomorrow.

@balmas
Copy link
Member

balmas commented Nov 15, 2018

I think, unspecific to the Safari problem, we do still need additional specificity in some of our styles, because occasionally underlying page styles are still bleeding through. So, some additional cleanup and reorganization of styles generally might help with that .

@kirlat
Copy link
Member

kirlat commented Nov 15, 2018

That's maybe not related to the topic directly, but since we're discussing CSS organization and processing I think it might be worth discussing here. The question I want to bring is whether a UIKit is an asset or a liability in our current design.

I have a feeling we do not follow UIKit styles extensively throughout our apps because our custom styles (and our style requirements) are so different from what UIKit is now. So would there be any value of continuing using a UIKit? This is an extra dependency and extra CSS weight we have to carry.

AFAIR we're using UIKit to style just a couple of elements such as buttons and for those we can copy out UIKit styles and make our own out of them. I also see no value in UIKit JS because we're using Vue.js instead (that's a change from before where UIKit JS interactions could be valubale). Dropping UIKit may also simplify some styling where we do override UIKit styles; we could remove unnecessary selectors if we won't have a UIKit on board. It's always better to make things simpler 🙂

I don't have any strong opinion on this, but I think it might be worth discussing here. So I'm wondering what do you think.

@irina060981 - moved it to alpheios-project/documentation#4 (comment)

@balmas
Copy link
Member

balmas commented Nov 15, 2018

I was wondering that too, as I looked at the styles today. I think it might be more of a liability right now. Simplification prior to implementing a design refresh in coming months is also probably a good idea.

@irina060981 - moved it to alpheios-project/documentation#4 (comment)

@irina060981
Copy link
Member

@irina060981
Copy link
Member

irina060981 commented Nov 17, 2018

Fixes for the issue:
poesialatina.it (buttons styles)
Firefox
image

Safari
screen shot 2018-11-17 at 19 18 57

@balmas
Copy link
Member

balmas commented Nov 19, 2018

ready for testing in build 2.1.0.9

@monzug
Copy link
Author

monzug commented Nov 19, 2018

Hello Irina (@irina060981 ),
Bridget suggested that this fix might be the cause of the inflection tables messed up in latest build 2.1.0.9.
see screenshot
screen shot 2018-11-19 at 4 44 55 pm

@monzug monzug reopened this Nov 19, 2018
@balmas
Copy link
Member

balmas commented Nov 19, 2018

@irina060981 I was thinking this was the removal of the flex styles, but I see that you are right that the class that you removed wasn't used so it must be something else

@balmas
Copy link
Member

balmas commented Nov 19, 2018

Another fairly big issue that I didn't think of before (I'm so sorry!) with the use of alpheios-popup and alpheios-panel as IDs in the class hierarchy: the UI controller design intentionally allows us the enclosing application to define the ids of these elements. So for the embedded library, we use alpheios-popup-embedded and alpheios-panel-embedded. Originally this was to avoid conflicts between the installed extension and the embedded library. We handle this differently now, but I think maybe we still want to allow this level of flexibility. Also, an outstanding feature request is to allow for multiple popups on the page, and if we use a fixed ID we can't do that.

So I think our choices are to make the top level of the hierarchy a class rather than an id, or maybe to do as Kirill was talking about, and use a custom element as the top level element of the popup or panel, rather than a div. According to the MDN though, I'm not sure how well Safari supports this standard (https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements).

@irina060981
Copy link
Member

irina060981 commented Nov 20, 2018

Hello, @monzug and @balmas !
I am very sorry, that I missed the issue with inflection tables while testing my changes.
I have made an investigation - what is happening with panel width and inflection table layout.
And it is a curious problem here - it is the problem from how Safari set priority to styles.
Both problems (panel width and grid layout) were from redefining styles writing inline styles - and they have less priority, than we were expected (from css styles with important)
So I have solved it by two steps:

  1. I have removed default width for panel from styles and placed it to inline styles
  2. I have excluded grid-template-columns from styles to that we added important flag

The same here #136 (comment)

Now it looks ok:
Safari
screen shot 2018-11-20 at 21 47 39

@irina060981
Copy link
Member

About wrapping inside ID - yes, Bridget, I have forgotten about this flexibility too.
As for Safari issues - there is no need in adding ID attribute, important flag is quite enough.
About others - I think we should spend more time on different variants of styles.
Then I will remove ID wrapping from sass.

@kirlat
Copy link
Member

kirlat commented Nov 20, 2018

So I think our choices are to make the top level of the hierarchy a class rather than an id, or maybe to do as Kirill was talking about, and use a custom element as the top level element of the popup or panel, rather than a div. According to the MDN though, I'm not sure how well Safari supports this standard (https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements).

There are custom elements that are part of webcomponents specification. Unfortunately, they are no go for Safari and MS Edge. Even in FF it is still in development: https://caniuse.com/#feat=custom-elements. But that's much more than we need because webcomponents also present a JS API that we're not going to use for this task.

However, if I understand correctly, we can just use our own HTML tags, without any webcomponents functionality. It may be not valid HTML5 (not sure would it or wouldn't it be, but our injected code would probably never be validated according to HTML5 specs), but should work in any browse, as far as I know:
https://stackoverflow.com/questions/2802687/is-there-a-way-to-create-your-own-html-tag-in-html5

The terminology is confusing as both terms are so close. We cannot use custom elements now, but can use custom tags.

It we'll need this feature, we can do an easy test in Safari: add our own element to the page, style it with CSS, and check the result.

@balmas
Copy link
Member

balmas commented Nov 20, 2018

If we can get Safari working well enough with a majority of sites without using a custom tag, I would prefer that for now. We will likely need to revisit this with the upcoming design refresh.

@irina060981
Copy link
Member

merged and closed

@balmas
Copy link
Member

balmas commented Nov 21, 2018

this can be tested in build 2.1.0.10

@monzug
Copy link
Author

monzug commented Nov 27, 2018

tested in Safari build 2.1.0.11 - looks good, checked several sites so far so good

@monzug monzug removed their assignment Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug safari safari-specific issues verified
Projects
None yet
Development

No branches or pull requests

4 participants