1094320 - It's impossible to have rich HTML markup inside heading elements (h1, h2, h3, h4) if they defined inside gaia-header component

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Oleg Zasypkin [:azasypkin]]

In bug 1080828 we use <bdi> elements inside <h1>, so that we have something like this:

<gaia-header><h1><bdi>Bidirectional text</bdi></h1></gaia-header>

At some point gaia-header changes this markup to: 

<gaia-header><h1>Bidirectional text</h1></gaia-header>

After quick debugging it seems that culprit is [1].

[1] https://github.com/mozilla-b2g/gaia/blob/7918024c737c4570cacd784f267e28737ae05dea/shared/elements/gaia-header/gaia-header.js#L223

Comment 1

[Oleg Zasypkin [:azasypkin]]

Hey Wilson,

Could you please look into this?

Thanks!

Comment 2

[Wilson Page [:wilsonpage]]

I don't think it will be possible to support nested elemenets inside <h1> in gaia-header. The text positioning logic is super complex, and relies on this assumption.

Have you tried using CSS (unicode-bidi: isolate) [1].

[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/bdi

Comment 3

[Oleg Zasypkin [:azasypkin]]

(In reply to Wilson Page [:wilsonpage] from comment #2)
> I don't think it will be possible to support nested elemenets inside <h1> in
> gaia-header. The text positioning logic is super complex, and relies on this
> assumption.
> 
> Have you tried using CSS (unicode-bidi: isolate) [1].
> 
> [1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/bdi

Our real use case is a bit more complex than one I mentioned in comment 0:

<gaia-header>
  <h1>
   <bdi>Localizable_Term</bdi>
   <span class="some-class" dir="ltr">&nbsp;‎(+3)</span>
  </h1>
</gaia-header>

So probably we can workaround "some-class" logic, not sure about bidirectional content inside, we'll see if we can use left-to-right mark here instead (though using marks looks like a hack to me).

Julien, do you have any other ideas how to solve this?

Comment 4

[Oleg Zasypkin [:azasypkin]]

Unfortunately it seems impossible to achieve effect we want with LTR and RTL marks only.

See "FirstName LastName (+1)" example in bug 1080828 comment 3 to understand what we're trying to display in the heading. To display it correctly we need to somehow add dir="auto" for the "FirstName LastName" part.

Comment 5

[Julien Wajsberg [:julienw]]

Hey Wilson, can you explain why you just don't call runFontFit (or even fontFit.reformatHeading) in rerunFontFit?

The font fit algorithm works fine for us even with nested elements (obviously they need to be inline and without fancy css markup), it's only this line that flattens the elements.

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Hey Wilson, can you explain why you just don't call runFontFit (or even
> fontFit.reformatHeading) in rerunFontFit?
> 
> The font fit algorithm works fine for us even with nested elements
> (obviously they need to be inline and without fancy css markup), it's only
> this line that flattens the elements.

Ok, forget what I say here, it doesn't work fine with nested elements currently. Maybe it's possible to make it work with inline nested elements though?

Comment 7

[Wilson Page [:wilsonpage]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Hey Wilson, can you explain why you just don't call runFontFit (or even
> fontFit.reformatHeading) in rerunFontFit?
> 
> The font fit algorithm works fine for us even with nested elements
> (obviously they need to be inline and without fancy css markup), it's only
> this line that flattens the elements.

I understand the problem you're facing now :)

We don't have a public `.runFontFit()` method currently. I think we do need it due to the number of cases in Gaia that require this feature. I remember :gmarty had issues with us exposing this, IMO the mutation observers just aren't good enough solution, and calling them via reseting `textContent` is a hacky solution that I'm forever having to explain to people :(

NI :gmarty to help us come to a solution.

Comment 8

[Julien Wajsberg [:julienw]]

Note that I removed the textContent reseting line as part of my WIP patch in https://github.com/mozilla-b2g/gaia/pull/26194, it seems to work fine (but nested markups still don't work properly and I still have other issues though ;) ).

I see "runFontFit" is called directly in the music application, do you know if it's been done on purpose? (I think you're actually the author of the patch, Wilson :) )

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: github PR

I looked with Guillaume and we found the issue.

So it looks like we have a lot of spaces in the second <bdi> element, and gaia-header does not trim or squash the spaces. So the patch does this and change the "textContent = textContent" call to a direct call to reformatHeading.

Unit tests are not finished yet.

We should check why we have so much spaces in this element though.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8525503 [details] [review]
github PR

Hey Guillaume, I think it's ready!

Maybe you'd prefer that I ask the review to Wilson?

Comment 11

[Guillaume Marty [:gmarty]]

Comment on attachment 8525503 [details] [review]
github PR

Looks good to me.
I'm happy to see that component getting more stable!

Comment 12

[Julien Wajsberg [:julienw]]

next steps:

To stamp a new version: `bower version patch && git push upstream master --tags`
To land in Gaia: We land to gaia-components, stamp a version and in Gaia `bower update gaia-header`.

Comment 13

[Julien Wajsberg [:julienw]]

Landed in gaia-header/master: cacec75985511965d5b3a4e2b199d1ca28cee5df

PR for the gaia change: https://github.com/mozilla-b2g/gaia/pull/26696

Comment 14

[Julien Wajsberg [:julienw]]

All green, landed on Gaia master: 2cb461dde5b9430ed87919e768de8c1cea503db6