Created attachment 124094 [details] [review]
fix rtl problems in xsl

patch -p1

Created attachment 124095 [details]
yelp showing the problem before the fix

notice the prev/next arrows and the icon

Created attachment 124096 [details]
fixed

Bingo!  I've been meaning to do something along these lines for a while.  Some comments:

* We can shave off some time by reducing the number of calls we effectively make to l10n.gettext.  At the beginning of db2html.css.content, we could precompute direction, align.start, align.end, etc and put them in variables.  Then, when we need them, we can just do xsl:value-of on the variable.  In precomputing the various direction-dependent things, pass the precomputed direction parameter.  That way we should end up with only one call to l10n.gettext.

* Let's change l10n.after.text to l10n.arrow.next and l10n.before.text to l10n.arrow.previous.  I think that's more clear.

* This isn't right:
-  background-position: 6px 0.5em;
+  background-position: </xsl:text><xsl:call-template name="l10n.pos.start"/><xsl:text>; 0.5em;
First, it has an extra semicolon that will effectively drop the vertical position.  Second, it loses the 6px of background padding.  We may need to rework the HTML output for admonitions to get this one right.

* I don't think we l10n.pos.start and l10n.pos.end.  We can just use the 'left' and 'right' we get from l10n.align.start and l10n.align.end instead.

* Do HTML renderers reverse table layouts automatically in RTL languages?  You've fixed the stuff we do for .td-first, but I want to be sure that .td-first is actually right-most when rendered for you.

* I notice you've added direction:ltr for span.code and span.command.  Given DocBook's massiveness, there are probably dozens of other things you want to do that for: classname, computeroutput, function, etc, etc, etc.

* For the new templates, please add parameter descriptions and content.  See, for instance, the definition of l10n.direction for this.  The full content can be something as simple as 'REMARK: Add some documentation'.  But the build won't pass 'make check' if there's nothing there.

* Nitpicky, but please insert two blank lines before the xsldoc comment that introduces each template.

* Also nitpicky: Indent the xsl:when and xsl:otherwise elements another two spaces in from their surrounding xsl:choose elements.


This is really nice work, and I'd like to get it committed soon.  It might be easier to submit a patch without the admonition fix in, so that we can get the bulk of the work committed.  Then we can work on the XSLT changes necessary to get the alignment of the admonition graphics right.

One more comment: In span.code and span.command, both left and right padding are 0.2em, so there's really not much point in calling l10n.aling.start and l10n.align.end.  Just leave the CSS directives as-is and delete my "FIXME: rtl" comment.  (I think I just indiscriminantly put that comment above all left and right directives, without thinking about any of them much.)

thank you for your kind respond
> * We can shave off some time by reducing the number of calls we effectively
make to l10n.gettext

I'm not familiar with gettext in xml
I don't know how todo that, but to make it more optimized 
I made another patch that uses params that are calculated once
in a way similar to what I did in docbook-xsl 

https://bugzilla.redhat.com/show_bug.cgi?id=475077

please feel free to choose or edit them the way you like
either do it the gettext way or use the new patch

> * Let's change l10n.after.text to l10n.arrow.next and l10n.before.text to
done
but if you going the gettext approach, then make this one that way too.

* This isn't right:
ok about ; this is trivial but the 6px can't be fixed
if it was a percent I could do something like
</xsl:text><xsl:call-template name="l10n.pos"><xsl:with-param name="x" select="0"/>%</xsl:call-template><xsl:text>

<!--**==========================================================================
l10n.pos
Extracts the position percent from the native direction corner ($x=6 returns 6 for English and 94 for Arabic)
$x: The x coordinate percentage.
-->
<xsl:template name="l10n.pos">
  <xsl:param name="x"/>
  <xsl:param name="direction">
    <xsl:call-template name="l10n.direction"/>
  </xsl:param>
  <xsl:choose>
  <xsl:when test="$direction = 'rtl'"><xsl:value-of select="100-$x"/></xsl:when>
  <xsl:otherwise><xsl:value-of select="$x"/></xsl:otherwise>
  </xsl:choose>
</xsl:template>

of if we could know the width we can replace 100 with the know width
but it can't be done that way (at least I can't do it)

so either accept it with no padding or but it inside a table or a div with the required padding

but it's not only ugly to be always on left but it unreadable for Arabic docs
please accept it and surround it with a box with the needed pading

> * I don't think we l10n.pos.start and l10n.pos.end.  We can just use the 'left'
> and 'right' we get from l10n.align.start and l10n.align.end instead.
it's your call

> * Do HTML renderers reverse table layouts automatically in RTL languages? 
yes, if you gave me a sample table I'll test it

> * I notice you've added direction:ltr for span.code and span.command.  Given
yes I also add it to pre unconditionally so it will work
maybe I'll add it to code and tt unconditionally

thanks for your kind response

Created attachment 124162 [details] [review]
optimized fix rtl problems (patch -p1)

Created attachment 124169 [details] [review]
optimized fix rtl problems (patch -p1)

same as the previous but with documentation

Actually, I preferred the use of templates as in your first patch.  I guess I wasn't clear on what I meant with using variables.  What you had in gettext.xsl was fine (except for the other nitpicks I mentioned).  What I wanted was to start db2html.css.content off like this:

<xsl:template name="db2html.css.content">
  <xsl:variable name="direction">
    <xsl:call-template name="l10n.direction"/>
  </xsl:variable>
  <xsl:variable name="align.start">
    <xsl:call-template name="l10n.align.start">
      <xsl:with-param name="direction" select="$direction"/>
    </xsl:call-template>
  </xsl:variable>
...

Then throughout db2html.css.content, just use these local variables.  The call to l10n.direction results in a call to l10n.gettext.  Calls to l10n.align.start will result in a call to l10n.gettext if the direction parameter isn't passed explicitly.  Doing it this way means db2html.css.content only calls l10n.gettext once.

I just changed the way admonitions are handled in a way that should make those CSS changes much easier.

I just changed db2html-inline.xsl to set the dir attribute on spans based on a number of criteria.  Most importantly, if the language attribute is set on the source element, you'll get the dir attribute appropriate for that language.  Otherwise, a set number of elements default to ltr:

http://svn.gnome.org/viewvc/gnome-doc-utils/trunk/xslt/docbook/html/db2html-inline.xsl?r1=1131&r2=1129&pathrev=1130

This should help with mixed-language documents.  Obviously, the same sort of stuff needs to be done in db2html-block.xsl and elsewhere.  Would you mind reviewing the above diff to make sure the elements I defaulted to ltr are appropriate?

what should I do now ?
could you please take this bug as yours

do it the way you like
so that in gnome 2.25 it will be fixed

the solution in the first patch is some how unique
you can use sed or perl to automate the replace

perl -wpe 's|\<xsl:call-template name="l10n\.align\.end"/\>|<xsl:value-of select="\$align.end"/>|' rtl.patch

or something like that

I guess it should not be local
because after we fix this we need to fix yelp package

the toc generated by yelp have problems with rtl and unlike this package there are no comments and it seems that there is an external hardcoded css file

Muayyad, I am working on this, as time permits.  Your patch was an excellent start, but there's quite a bit more that needs to happen.  I'd like to get all the RTL issues resolved for Gnome 2.26.

Note to self: We should also change the direction of the arrow used for menuchoice elements.

Thanks, I did not meant to be demanding. TYT

I just wanted to make sure that you know that my patch does not fix yelp toc
and that yelp toc does have problems with RTL

is yelp on you hands too, or should I notify its developer ?

when ever you want to a hand or testing, I'm there waiting to help, email me or add notice here, we could also set a time on IRC or what ever way you like.

Sorry for the delay on this.  I've just committed the changes and released gnome-doc-utils 0.15.1.  It would be appreciated if people could test this before 2.26.0.  I'm going to leave the bug open for now until we've shaken out any further problems.

We should open a separate bug for yelp problems and make it depend on this bug.

More fixes applied in r1149

could you please pass me the url or svn command to get those fixes and test them

yelp module in GNOME SVN. See http://live.gnome.org/SVN

For this bug, the gnome-doc-utils module is what you want.

http://svn.gnome.org/viewvc/gnome-doc-utils/trunk/

There are a number of affected files, but to test out, you just need to process a DocBook file with xslt/docbook/html/db2html.xsl.

If you install gnome-doc-utils, either from SVN or from the latest unstable tarball, you can just use gnome-doc-tool.

gnome-doc-tool html -o html/ /path/to/docbook.xml

removing target as discussed on irc with shaun.

Pretty sure all the RTL issues are fixed. Please file a new bug if there are more problems.