Oleg Zasypkin [:azasypkin] Reporter
	Description • 10 years ago

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

	Oleg Zasypkin [:azasypkin] Reporter
	Comment 1 • 10 years ago

Hey Wilson,

Could you please look into this?

Thanks!

	Wilson Page [:wilsonpage]
	Comment 2 • 10 years ago

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

	Oleg Zasypkin [:azasypkin] Reporter
	Comment 3 • 10 years ago

(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?

	Oleg Zasypkin [:azasypkin] Reporter
	Comment 4 • 10 years ago

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.

	Julien Wajsberg [:julienw] Assignee
	Comment 5 • 10 years ago

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.

	Julien Wajsberg [:julienw] Assignee
	Comment 6 • 10 years ago

(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?

	Wilson Page [:wilsonpage]
	Comment 7 • 10 years ago

(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.

	Julien Wajsberg [:julienw] Assignee
	Comment 8 • 10 years ago

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 :) )

	Julien Wajsberg [:julienw] Assignee
	Comment 9 • 10 years ago

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.

	Julien Wajsberg [:julienw] Assignee
	Comment 10 • 10 years ago

Comment on attachment 8525503 [details]
github PR

Hey Guillaume, I think it's ready!

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

	Guillaume Marty [:gmarty]
	Comment 11 • 10 years ago

Comment on attachment 8525503 [details]
github PR

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

	Julien Wajsberg [:julienw] Assignee
	Comment 12 • 10 years ago

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`.

	Julien Wajsberg [:julienw] Assignee
	Comment 13 • 10 years ago

Landed in gaia-header/master: cacec75985511965d5b3a4e2b199d1ca28cee5df

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

	Julien Wajsberg [:julienw] Assignee
	Comment 14 • 10 years ago

All green, landed on Gaia master: 2cb461dde5b9430ed87919e768de8c1cea503db6