classesStyles properties don't override baseFontStyle properties #102

yamov · 2018-02-12T23:06:29Z

classesStyles properties override the corresponding baseFontStyle properties only if the tagsStyles have the same properties.

For example:

<HTML
    html={'Just text <p>Normal paragraph</p><p class="smallRedFont">Small red paragraph</p>'}
    baseFontStyle={{ fontSize: 20, color: 'green' }}
    tagsStyles={{ p: { lineHeight: 30 } }}
    classesStyles={{ smallRedFont: { fontSize: 10, color: 'red' } }}       
 />

fontSize and color properties of smallRedFont class won't override fontSize and color of baseFontStyle, because tagsStyles for p doesn't include these properties.

In this case, however,

<HTML
    html={'Just text <p>Normal paragraph</p><p class="smallRedFont">Small red paragraph</p>'}
    baseFontStyle={{ fontSize: 20, color: 'green' }}
    tagsStyles={{ p: { fontSize: 16, color: 'blue' } }}
    classesStyles={{ smallRedFont: { fontSize: 10, color: 'red' } }}       
 />

fontSize and color properties of smallRedFont class will override fontSize and color of baseFontStyle, because tagsStyles for p does have these properties.

The text was updated successfully, but these errors were encountered:

Exilz · 2018-02-13T09:23:59Z

Hi, thanks for the very precise explanation and reproduction of this issue.

Could you try replacing the line 444 from HTML.js

from :

const textElementStyles = this.filterBaseFontStyles(element, classStyles, props);

to

const textElementStyles = parentTag && TEXT_TAGS.indexOf(parentTag) !== -1 ? {} : this.filterBaseFontStyles(element, classStyles, props);

Basically, what I've done here is not to apply baseFontStyle to text nodes nested inside other text nodes, since the parent should be the one receiving the style anyway.

I'm pretty concerned with regressions, and the demo isn't complete enough for me to check all use cases. I'd like you to tell me if it fixed it for you, and if you see any potential drawbacks.

yamov · 2018-02-14T13:16:00Z

Hey, @Exilz

Your solution doesn't seem to work properly :/.
It solves the case of classesStyles but breaks some other cases. Namely, it seems like baseFontStyle does not apply to tags (without classes) anymore.

Exilz · 2018-03-23T09:36:43Z

@yamov @fvsch

Allright, so I took some more time to dig into this problem, which seems closely tied to #119 and it's way more complicated than what I originally thought.

I think the whole logic of text style inheritance needs to be rewritten, filterBaseFontStyle and those overridenFrom... variables just don't cut it.

Here's the logic I came up with that I need to try out :

DON'T apply any styling to the text nodes that aren't mapped as rawtext, only compute the final styles for each rawtexts. This just seems easier overall.
For each rawtext, recursively loop from the deepest text parent to the highest.
Create an empty object that will receive the final text styles for this rawtext
On each iteration, create a style object with the proper priority order : htmlAttribs > classesStyles.class > tagsStyles.tag > baseFontStyle
Only merge the keys that aren't yet applied to the final object. ie: if fontSize is already set in the first iteration, ignore the fontSize that we got from the 3rd iteration because of a class for instance.

Let's take this snippet as an example :

<p>
    Texte paragraphe
    <span class="red-p">red span </span>
    <span class="blue-p">
         blue<em> span</em>
    </span>
</p>

And those props :

            baseFontStyle: { fontSize: 20, color: 'green' },
            classesStyles: {
                'red-p': { fontSize: 10, color: 'red' },
                'blue-p': { fontSize: 10, color: 'blue' }
            }

If I'm not mistaken, the above algorithm should map this HTML to the following native components :

                <Text paragraph>
                    <Text raw style={{ fontSize: 20, color: 'green' }}>Texte paragraphe</Text>
                    <Text span>
                        <Text raw style={{ fontSize: 10, color: 'red' }}>red span</Text>
                    </Text>
                    <Text span>
                        <Text raw style={{ fontSize: 10, color: 'blue' }}>blue</Text>
                        <Text em>
                            <Text raw style={{ fontSize: 10, color: 'blue', fontStyle: 'italic' }}>span</Text>
                        </Text>
                    </Text>
                </Text>

I'll try to implement this and keep you posted.

Exilz · 2018-03-23T11:20:54Z

Holy moly, I think I'm getting there. I'll create a separate branch so you can take a look at it and test it yourselves. This won't be part of the next release since this is quite a major rewrite.

This should be a much needed improvement addressing some of the oldest issues of this plugin. The new algorithm to be tried is explained [here](#102 (comment) ssuecomment-375594792)

Exilz · 2018-03-23T16:26:11Z

A first version of these changes is available for testing in the development branch. I'm looking forward to your feedback.

fvsch · 2018-03-23T16:55:41Z

Thanks, I’ll take a look on monday.

CarstenHoyer · 2018-04-10T14:02:57Z

@Exilz I had a related problem using version 3.9.1. fontSize would not apply, when I was using classesStyles. Here is a test to reproduce it:

The test branch works:
"react-native-render-html": "https://github.com/archriss/react-native-render-html#2fc1f8af99aeb76df1e5816a1d629b27058606ad",

const alterNode = (node) => {
    if (node.name === 'p' && !node.parent) {
      node.attribs.class = 'blue-p'
    }
  }

  return (
    <HTML
      baseFontStyle={{ fontSize: 20, color: 'green' }}
      alterNode={alterNode}
      classesStyles={{
        'blue-p': { fontSize: 100, color: 'blue' }
      }}
      html={`<p>Testing</p>`}
    />
  )

Exilz · 2018-04-10T16:21:04Z

@CarstenHoyer glad to hear the development branch fixed this for you. I have been testing these changes on our applications, and it looks like it fixed a lot of wrong behaviors.

However, please note this broke some renderers that shouldn't have been working before but were using bugs to their advantage.

I'll be releasing this soon, along with some small new features.

More reviews are always welcome :)

theill · 2018-04-19T22:05:00Z

I've tested this as well and the dev branch works nicely for me.

This should be a much needed improvement addressing some of the oldest issues of this plugin. The new algorithm to be tried is explained [here](meliorence#102 (comment) ssuecomment-375594792)

This should be a much needed improvement addressing some of the oldest issues of this plugin. The new algorithm to be tried is explained [here](meliorence/react-native-render-html#102 (comment) ssuecomment-375594792)

yamov changed the title ~~classesStyles properties not always override baseFontStyle properties~~ classesStyles properties don't override baseFontStyle properties Feb 12, 2018

Exilz added the bug Crush'em all. label Feb 13, 2018

Exilz added the is:waiting for feedback Waiting for OP input. label Apr 5, 2018

Exilz mentioned this issue May 7, 2018

v3.10.0 #146

Merged

Exilz closed this as completed in #146 May 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

classesStyles properties don't override baseFontStyle properties #102

classesStyles properties don't override baseFontStyle properties #102

yamov commented Feb 12, 2018 •

edited

Loading

Exilz commented Feb 13, 2018

yamov commented Feb 14, 2018 •

edited

Loading

Exilz commented Mar 23, 2018

Exilz commented Mar 23, 2018

Exilz commented Mar 23, 2018

fvsch commented Mar 23, 2018

CarstenHoyer commented Apr 10, 2018

Exilz commented Apr 10, 2018

theill commented Apr 19, 2018

classesStyles properties don't override baseFontStyle properties #102

classesStyles properties don't override baseFontStyle properties #102

Comments

yamov commented Feb 12, 2018 • edited Loading

Exilz commented Feb 13, 2018

yamov commented Feb 14, 2018 • edited Loading

Exilz commented Mar 23, 2018

Exilz commented Mar 23, 2018

Exilz commented Mar 23, 2018

fvsch commented Mar 23, 2018

CarstenHoyer commented Apr 10, 2018

Exilz commented Apr 10, 2018

theill commented Apr 19, 2018

yamov commented Feb 12, 2018 •

edited

Loading

yamov commented Feb 14, 2018 •

edited

Loading