alpheios-project / webextension

Alpheios Browser Extensions
ISC License
6 stars 2 forks source link

Safari: look of the extension in some sites #135

Closed monzug closed 5 years ago

monzug commented 6 years ago

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

2) in http://thelatinlibrary.com/plautus/mercator.shtml, the Reskin options in Options panel are not alligned

irina060981 commented 6 years ago

the same as for https://github.com/alpheios-project/webextension/issues/136

Different priority for extension styles.

irina060981 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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

irina060981 commented 5 years ago

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

kirlat commented 5 years ago

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 commented 5 years ago

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

balmas commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/alpheios-project/documentation/issues/4#issuecomment-439603183

balmas commented 5 years ago

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 https://github.com/alpheios-project/documentation/issues/4#issuecomment-439603183

irina060981 commented 5 years ago

https://github.com/alpheios-project/node-build/pull/10 https://github.com/alpheios-project/webextension/pull/165 https://github.com/alpheios-project/components/pull/282

irina060981 commented 5 years ago

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

Safari

screen shot 2018-11-17 at 19 18 57
irina060981 commented 5 years ago

http://thelatinlibrary.com/plautus/mercator.shtml (reskin buttons) Safari

screen shot 2018-11-17 at 19 20 14
irina060981 commented 5 years ago

Merged and closed

balmas commented 5 years ago

ready for testing in build 2.1.0.9

monzug commented 5 years ago

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

balmas commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/alpheios-project/webextension/issues/136#issuecomment-439957205

Now it looks ok: Safari

screen shot 2018-11-20 at 21 47 39
irina060981 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

merged and closed

balmas commented 5 years ago

this can be tested in build 2.1.0.10

monzug commented 5 years ago

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